Prevent duplicate Ads conversion tracking ID output

[nfmohit]

Bug Description

While #8313 implements an independent ability for the Ads module to output the Ads conversion tracking ID, it doesn't prevent the Analytics module from still outputting this tag if still set.

We have #8248 that migrates the Ads conversion tracking ID from the Analytics to the Ads module thus effectively removing it from the Analytics module, however, keep in mind that this migration is conditional. This migration will only happen if the user goes to Site Kit Settings and opens the Analytics settings edit/view screens. See this AC from #8248:

For users who were previously using the value of the GA4 Ads Conversion ID label:

• Upon visiting the legacy settings field for the first time following plugin update, the value should be migrated to the new location applicable to the settings defined in the Ads module

Let's take the following scenario into consideration:

1. A pre-existing Site Kit installation has the Analytics module set up with an Ads conversion tracking ID entered.
2. Now, the new "Ads" module is launched.
3. The user does not feel any necessity to open the Analytics settings edit/view screens, as a result, the migration of the existing Ads conversion tracking ID does not happen from the Analytics to the Ads module.
4. They see the new "Ads" module and set it up with the same or different Ads conversion tracking ID.

The above steps will make Site Kit output two tags for the Ads conversion tracking ID, one from the Analytics module, and the other from the Ads module. Here's a screenshot of an experiment I did to replicate the above scenario:

I think the above scenario can potentially cause duplicate/erroneous tracking, and should be addressed.

Steps to reproduce

1. Set up Site Kit and the Analytics module.
2. Do not turn on the adsModule feature flag yet.
3. Add an "Ads Conversion ID" to the Analytics module.
4. Now, go back to the Site Kit settings (with Analytics settings accordion closed).
5. Turn on the adsModule feature flag.
6. Do not open the Analytics settings edit/view screens after this point, otherwise, the migration will be performed.
7. Go to Site Kit settings -> Connect More Services and connect the Ads module with a different "Conversion Tracking ID`.
8. View page source of the front end and notice that two ads conversion tracking IDs are placed.

---

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

Acceptance criteria

• There should only be one Ads tag rendered on the page for users of prior GA4 tracking and newly added Ads module
  • Users who do not visit the GA4 settings screen should not have duplicate gtags printed in the source
  • The (new) Ads module Conversion ID value should be used for gtag printing
  • The (legacy) GA4 module Conversion ID value should be ignored if an Ads equivalent is present

Implementation Brief

Version Specific Migration

• [ ] Create a new Migration_n_e_x_t.php (replacing n_e_x_t with the version this feature is expected to be released) which always migrates the adsConversionID from the Analytics Module to the Ads Module, if found, and activates the Ads Module. Use the includes/Core/Util/Migration_1_123_0.php file as the base structure of the migration.
  • [ ] Set the DB_VERSION property in the Migration class to the version in which this feature will be deployed.
  • [ ] Create $context, $options, $ads_settings and $analytics_settings class properties and in the constructor, set them in the same way as in the 1.123.0 migration, with the addition of the Google\Site_Kit\Modules\Ads\Settings class which should be included and instantiated to $ads_settings property.
  • [ ] The migrate method should check the current database version using $this->options->get( self::DB_VERSION_OPTION ); and if this value is falsy or below self::DB_VERSION using the version_compare function then it should run two migration methods:
    • $this->migrate_analytics_conversion_id_setting();
    • $this->activate_ads_module();
  • In the migrate_analytics_conversion_id_setting method:
    • [ ] Return null if ! $this->analytics_settings->has().
    • [ ] Get the settings from the analytics-4 module using $this->options->get( Analytics_4::MODULE_SLUG );
    • [ ] If there the adsConversionID setting exists and is not empty on the analytics-4 module:
      • [ ] Check if there is an existing adsConversionID on the Ads module, in this case, remove the value from the Analytics module settings using $this->analytics_settings->delete('adsConversionID').
      • If there is not an existing adsConversionID in the ads module:
        • [ ] Set the adConversionID on the Ads module settings using $this->ads_settings->set('adsConversionID', $analytics_settings['adsConversionID'])
        • [ ] Set the adsConversionIDMigratedAtMs on the Ads module settings to the current timestamp, in order to show the banner in the frontend later, using $this->ads_settings->set('adsConversionID', time() * 1000)
        • [ ] Remove the value from the Analytics module settings using $this->analytics_settings->delete('adsConversionID').
  • In the activate_ads_module method:
    • [ ] Check if the Ads module is already active by looking for the ads slug in the $active_modules = $this->options->get( 'googlesitekit-active-modules' ); array.
    • [ ] Check if the adsConversionID has been added to the Ads module options in the migration_analytics_conversion_id_setting method, by getting the ads module options using $this->ads_settings->get('adsConversionID') and checking that it is truthy.
    • [ ] If both of these checks are met, activate the Ads module by adding the ads module key to the $active_modules array using $active_modules[] = Ads::MODULE_SLUG then updating the active modules using $this->options->set( Modules::OPTION_ACTIVE_MODULES, $option );

Remove Frontend Migration Logic

• [ ] Remove assets/js/modules/analytics-4/hooks/useMigrateAdsConversionID.js and both uses of the useMigrateAdsConversionID hook from Analytics SettingsView and SettingsEdit.

Update the Analytics Notification Logic

• Confirm that the assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js behaves as expected with the adsConversionIDMigratedAtMs setting now being added by the migration.

Test Coverage

• Remove assets/js/modules/analytics-4/hooks/useMigrateAdsConversionID.test.js.
• Add a thorough set of tests in tests/phpunit/integration/Core/Util/Migration_n_e_x_tTest.php that tests these key cases:
  • Migration where neither the Analytics or Ads module are active
  • Migration where the Analytics module is active but does not have an adsConversionID
  • Migration where the Analytics module is active and has an adsConversionID where:
    • The Ads module is inactive
    • The Ads module is active with an existing adsConversionID

QA Brief

Changelog entry

[nfmohit]

I wanted to find out how this problem came into existence in the first place.

This seems to be related to #8248, which took a different approach to migrating the adsConversionID setting from the Analytics to the Ads module than we had planned in the design doc.

The design doc planned for a version-specific database migration (like we recently did with the Singular Analytics Module) where if an adsConversionID was present in the Analytics module, it would be automatically and unconditionally migrated to the Ads module.

However, during the definition of #8248, it was decided that the migration would happen only when the user goes to Site Kit Settings and opens the Analytics settings edit/view screens. This would allow Analytics to still output the tag until the migration was done, after which Ads would take over. However, I don't think a scenario was taken into consideration where a user could connect the Ads module without the migration being performed, thus causing the duplicate tags as described in the issue description.

If a DB migration was done here instead, it would be guaranteed that the conversion ID, if exists, would only exist in the Ads module. As a result, outputting the Ads conversion ID tag could be solely a concern of the Ads module, and the entire mechanism to store and output an Ads conversion ID could be removed from the Analytics module. This would ensure that there would be no duplicate tags.

@10upsimon will be able to share details regarding this decision as soon as he's back from his time off, but I'm sure he had valid concerns with the original database migration plans, thus, choosing to do a conditional runtime migration instead. Based on the comment here, I think the decision was mostly influenced by the necessity to make the user aware of the location change of the Ads Conversion ID field.

I've drafted two potential paths, one of which we can take to remedy the reported problem in this issue:

1. Do the database migration as planned. This will make sure the Ads Conversion ID only exists in the Ads module, and we can let the Ads module solely handle its output.
   1. This will require us to handle the display of the Ads Conversion Tracking ID notice a little differently than how it is done today. Currently, we start showing it as soon as the migration is done for 28 days. What we instead can do is start showing it when the user first opens the Analytics settings edit/view screens after the DB migration is done and retain it for 28 days.
   2. This will also allow us to remove any handling of the Ads conversion ID from the Analytics module, thus ensuring a clean separation of concerns. This would include the module setting, the Web_Tag/AMP_Tag methods, and the Site Health debug value.
2. OR, in the Analytics module, we could check if the Ads module is already outputting the tag, and if so, prevent Analytics from doing so again.

@10upsimon & @aaemnnosttv: I'll leave the decision-making step to you. Thank you!

CC: @zutigrm

[mxbclang]

Per discussion in Slack, I've changed this to a P1 and removed it from Sprint 124 as we don't consider it to be a launch blocker for the Ads Module launch.

[aaemnnosttv]

@nfmohit @10upsimon I think a one time DB migration makes sense for the handling related to moving the conversion ID value between modules. The concern of a temporary notice within GA can be handled separately similar what we do for new badges, but the two don't need to happen at the same time. It's possible they could use some shared state but if we had to choose, I'd rather have the temporary notice triggered by the migration in the background rather than trigger the migration as a side-effect of viewing the module settings (even if that means the notice isn't seen right away).

[aaemnnosttv]

@10upsimon Let me know if you have any questions about the direction here 👍

[mxbclang]

@10upsimon As noted in Slack, I'm going to add a Next Up label here and plan that we'll work on this in Sprint 126. Can you please prioritize for definition? Thank you!

[tofumatt]

ACs are good, moving to IB 👍🏻

[benbowler]

I've created an IB based on all of the comments above taking the approach of creating a db migration which migrates the Ads Conversion ID if present in the Analytics module settings and activating the Ads module as well as setting the existing adsConversionIDMigratedAtMs key which will show the setting moved banner if the user views it within the next 28 days.

As mentioned above we could/should create a follow up ticket to refactor AdsConversionIDSettingsNotice to show for 28 days after the user first views this Analytics settings after this migration has taken place.

[tofumatt]

@benbowler Can you create that follow-up issue? Fine to leave an outline and move it to the triage/AC column 👍🏻

IB here looks good, I think it makes sense 🙂

IB ✅

[mohitwp]

QA Update ❌

@benbowler Because this ticket testing involves switching between versions and testing Ads conversion ID migration. I tested this multiple times on different sites and confirm that migration is not working. Note: While switching between different versions sometimes tester plugin behaving strange and showing ads module under "connect more services" tab even when Ads module feature flag is not enabled.

Issue 1> Analytics Module setup with Ads Conversion ID before migration. - FAIL

• Set up Analytics with conversion ID on older versions 1.23.1. (Do not enabled Ads module feature flag)
• Switch to dev environment. ( Expected : Ads modules Feature flag will auto enabled).
• Open Analytics settings.
• Conversion ID migration not happening.
• Set up Ads module.
• Check Source code.
• Both Analytics and Ads "Conversion ID's" are placed.

https://github.com/google/site-kit-wp/assets/94359491/67629f05-8b97-4a3c-969b-823503286e0a

https://github.com/google/site-kit-wp/assets/94359491/32fb050b-6e42-4951-84b8-0633411ad7d0

https://github.com/google/site-kit-wp/assets/94359491/2df31b7a-3d28-43e0-9601-ebef57a2ebc7

Issue 2 : Analytics Module and Ads Module set up, both with conversion IDs set, before migration. (in this case the Ads module conversion ID is retained.) - FAIL

• Set up Analytics with conversion ID on older versions 1.23.1. (Do not enabled Ads module feature flag)
• Now enable Ads module feature flag.
• Set up Ads module ( require to switch to 1.24.0 version to setup conversion ID under Ads module).
• Switch to dev environment. ( Expected : Ads modules Feature flag will auto enabled).
• 'Complete setup' CTA appear for Ads module.
• Enter Conversion ID and setup Ads module.
• Open Analytics settings - Analytics have different conversion ID.
• Check Source code.
• Both Analytics and Ads "Conversion ID's" are placed.

https://github.com/google/site-kit-wp/assets/94359491/81dad65d-789a-4b8b-b256-f7239aa4b2c0

Analytics Module not setup before migration. _ PASS

https://github.com/google/site-kit-wp/assets/94359491/e258d69b-1f56-4d3c-ac45-5418c5799f8d

[benbowler]

Hey @mohitwp, as mentioned in Slack (thread including video), the failures above were caused because the migration wasn't running due to the develop branch not having a version number set here inside the migration file:

https://github.com/google/site-kit-wp/blob/0504ed8e13e4b83a44ac6fbdb01ec2a9d57792fe/includes/Core/Util/Migration_Conversion_ID.php#L28-L31

These migrations only run if the database version (stored in the database: wp_options googlesitekit_db_version) is lower than the DB_VERSION value in the migration. If the DB_VERSION is n.e.x.t it won't run.

---

In order to QA this, you can use the WordPress Plugin File Editor tool to update the DB_VERSION in the migration. Here are the steps: (video)

The key steps are:

1. Switch to the previous version and setup the Analytics/Ads module as you need for the test
2. Switch to the develop branch version
3. Using the Tools > Plugin File Editor page in WordPress, edit the DB_VERSION to be 1.129.0 in google-site-kit/includes/Core/Util/Migration_Conversion_ID.php and click Update
4. Now visit the plugin dashboard - the migration will happen and you can verify the results in the settings

Note that if you want to test again you must reset the whole site (or change the googlesitekit_db_version value in the database back to an older version number) as this value in the settings is not reverted when you change branches with the tester plugin. Or as @wpdarren mentioned you can use the db version setting in the tester plugin.

[mohitwp]

QA Update ⚠️

• Tested on dev environment.
• Tested using Plugin Editor file tool as per steps mentioned by Ben.

@benbowler

I verified below listed scenarios and migration is executing successfully in below cases - But I noticed that migration notice is not appearing under the analytics settings view.

• Scenario 1 : Analytics Module setup with Ads Conversion ID before migration.
• Scenario 2 : Analytics Module and Ads Module set up, both with conversion IDs set, before migration. (in this case the Ads module conversion ID is retained.)

Scenario 1 : Analytics Module setup with Ads Conversion ID before migration.

https://github.com/google/site-kit-wp/assets/94359491/822f3963-50b4-4152-8c10-f321ec3f6f80

Scenario 2 : Analytics Module and Ads Module set up, both with conversion IDs set, before migration. (in this case the Ads module conversion ID is retained.)

https://github.com/google/site-kit-wp/assets/94359491/95470fc2-3db5-4a5e-9e05-848267ed5c6d

[benbowler]

Hey @mohitwp, really good catch here! I found the issue and have created a follow up PR where the notice should show correctly.

To be clear, in Scenario 2, this message should not show, because in this case the user has already visited and added the conversion ID to the Ads module themselves so the value hasn't actually been migrated and the banner wouldn't make sense.

For Scenario 1 this banner should show after the migration has occurred, until the user dismisses it or until 28 days after the migration.

[mohitwp]

QA Update ✅

• Tested on main environment.
• Verified scenarios mentioned under Ads module bug bash instructions.
• Verified that migration notice is now appearing under the analytics settings view.
• I verified below listed scenarios and migration is executing successfully in below cases - But I noticed that migration notice is not appearing under the analytics settings view.

Scenario 1 : Analytics Module setup with Ads Conversion ID before migration. - PASS Scenario 2 : Analytics Module and Ads Module set up, both with conversion IDs set, before migration. (in this case the Ads module conversion ID is retained.) - PASS Scenario 3: Only Ads Module is setup on older versions. - PASS

Scenario 1 : Analytics Module setup with Ads Conversion ID before migration. -

https://github.com/google/site-kit-wp/assets/94359491/4befaf40-4d58-4cb6-81c8-fb438c8ad934

Scenario 2 : Analytics Module and Ads Module set up, both with conversion IDs set, before migration. (in this case the Ads module conversion ID is retained.)

https://github.com/google/site-kit-wp/assets/94359491/5292e24b-cdd4-4617-9af7-45d3746e778d

Scenario 3: Only Ads Module is setup on older versions.

https://github.com/google/site-kit-wp/assets/94359491/18e6a27b-ad99-4415-a271-fb257fa94433

Both Analytics and Ads module connected

Only Ads module connected

AMP Analytics code when AMP plugin is active

[aaemnnosttv]

@nfmohit @benbowler let's follow up with an issue to rename this migration for consistency with the others. Thanks!

[benbowler]

Created a follow up ticket #8905