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

[bethanylang]

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 👍

[bethanylang]

@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 ✅