Update `Analytics_4` settings to include AdSense Link-related Settings fields (`adSenseLinked` and `adSenseLinkedLastSyncedAt`)

[tofumatt]

Feature Description

The settings adSenseLinked and adSenseLinkedLastSyncedAt should be added to the Analytics 4 settings and made available to the datastore. See: https://docs.google.com/document/d/1-JenwPTAkw0eTkmESzjTOfgphs9qpRYH0h3xk_s3a4o/edit#heading=h.w9m3017hbv7c

These settings should both be made available in the view-only context (eg. to users using Dashboard Sharing).

---

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

Acceptance criteria

• The settings adSenseLinked and adSenseLinkedLastSyncedAt should be added to Analytics 4 module (eg. includes/Modules/Analytics_4/Settings.php).
  • adSenseLinked should default to false, and should be set to true when Analytics and AdSense accounts are linked.
  • adSenseLinkedLastSyncedAt should default to null, and be a UNIX timestamp when synced.
  • Whenever the user's AdSense account or GA4 property changes, or when they disconnect AdSense module, we should reset both values to their original/default values.
• (These settings should be visible to users on the view-only dashboard as well.)

Implementation Brief

• [x] Ensure the adsenseLinked key is added to get_view_only_keys in Analytics_4
• [x] Add 'adSenseLinkedLastSyncedAt' => null to https://github.com/google/site-kit-wp/blob/8612fe95e03b6029aab6cd32c9df6afd1d860225/includes/Modules/Analytics_4/Settings.php#L90-L109
• [x] When AdSense is disconnected (https://github.com/google/site-kit-wp/blob/8612fe95e03b6029aab6cd32c9df6afd1d860225/includes/Modules/AdSense.php#L181-L191), reset both settings.
• [x] When the AdSense account (https://github.com/google/site-kit-wp/blob/8612fe95e03b6029aab6cd32c9df6afd1d860225/includes/Modules/AdSense/Settings.php#L181) or GA4 property (https://github.com/google/site-kit-wp/blob/8612fe95e03b6029aab6cd32c9df6afd1d860225/includes/Modules/Analytics_4/Settings.php#L96) changes, reset both settings. You can use the on_change handler at https://github.com/google/site-kit-wp/blob/53a1aa4ba8018ab8caf30ca4f3412689cee84929/includes/Modules/Analytics_4.php#L177-L185 to add conditions for GA4 property ('propertyID') changing. For AdSense, add a similar on_change handler for settings as is done in the Analytics 4 module in the register function, that checks for changes to 'accountID'.

Test Coverage

• Tests should verify the behaviour outlined in the ACs/approach in IB, especially around changing settings/AdSense module disconnection.

QA Brief

1. Set up Site Kit with the Analytics & AdSense modules.
2. In the SK dashboard, run the following in the browser console: googlesitekit.api.get( 'modules', 'analytics-4', 'settings', {}, { useCache: false } ).
   • This will trigger a network request to /modules/analytics-4/data/settings. Take a look at its response and verify that adSenseLinked & adSenseLinkedLastSyncedAt are available with false and 0 respectively as default values.
3. Run the following in the browser console: googlesitekit.api.set( 'modules', 'analytics-4', 'settings', { adSenseLinked: true, adSenseLinkedLastSyncedAt: 12345 } ).
   • This will trigger another POST network request to the same endpoint to set these settings to some arbitrary values. Look at the request response to verify that these values are set.
4. Now, using the Site Kit settings, change the Analytics property.
5. Perform # 2 again and verify that these settings have reverted to default false and 0 values respectively.
6. Perform # 3 again to set them to arbitrary values.
7. Even though it is not possible to change the AdSense account ID from SK settings and changing this is not practical, let's test this in a hacky way just to make sure the implementation meets the ACs.
   • In the browser console, run the following scripts:

     googlesitekit.data.dispatch( 'modules/adsense' ).setAccountID( "SOME OTHER ADSENSE ACCOUNT ID" );
     googlesitekit.data.dispatch( 'modules/adsense' ).saveSettings();

   • Refresh the page.
8. Perform # 2 again and verify that these settings have reverted to default false and 0 values respectively.
9. Perform # 3 again to set them to arbitrary values.
10. Using Site Kit settings, disconnect the AdSense module.
11. Perform # 2 again and verify that these settings have reverted to default false and 0 values respectively.
12. Share Analytics Module with another user, and login with that user
13. Go to the shared dashboard and perform # 2 again. Verify that adSenseLinked is present in response

Changelog entry

• Add AdSense link related settings to the Analytics module.

[tofumatt]

@kuasha420 Just wanted to check here, should we be storing the AdSense account ID that's linked to Analytics? Or just a boolean saying it's linked? I thought the ID was right but wanted to confirm 🤔

If this looks good feel free to move it to IB 😄

[kuasha420]

@tofumatt Thank you for the mention here. I think we can get away with a boolean here, however, we need to make sure to invalidate the timestamp and the adSenseLinked when the AdSense account changes, the GA4 property changes or the AdSense module is disconnected (the settings will be wiped when the Analytics module is disconnected), so that the sync logic runs again when the account is changed, similar to how the dataAvailable state is handled in #5933 . What do you think?

[tofumatt]

@kuasha420 That sounds good, I've updated the ACs and moved this to IB 😄

[tofumatt]

Leaving this as a 15 as I didn't notice a clear/straightforward way to detect changes to module settings in PHP, but maybe I just missed it? 😓

[techanvil]

Hey @tofumatt, a couple of points on the IB:

• It looks like adSenseLinkedLastSyncedAt should be added to get_view_only_keys() as well as adSenseLinked.
• We should be able to add conditions to the existing on_change handler for responding to the module setting changes.

~On more of an AC-related note though, it seems that we shouldn't actually need both adSenseLinked and adSenseLinkedLastSyncedAt, as we can derive the value of adSenseLinked from adSenseLinkedLastSyncedAt. It's a similar scenario to that discussed back on this thread, where we introduced keyMetricsSetupCompletedByUserID and removed keyMetricsSetupCompleted as a result.~

~So, I think we should probably revise the AC here to specify this and modify the IB accordingly. What do you think?~

[kuasha420]

@techanvil I may be wrong on this, but the Linked state can be either true or false after the sync, how would we derive either from the adSenseLinkedLastSyncedAt timestamp? The timestamp will be used to limit the API call here. We can only have one setting if we decide to keep the timestamp null until the linked status is true, but in that case, there will be API call on every load until the accounts are linked, and some user will never link them, resulting in excessive API requests. Am I missing something here?

[techanvil]

Hey @kuasha420, thanks for clarifying that. You are absolutely right, I had the wrong mental model of the sync process. Thanks for the catch! I'll strike that point from my review, and leave the other points for Matt to consider.

[tofumatt]

@techanvil The main use of adSenseLinkedLastSyncedAt is knowing when the last sync was to see if we should do another sync, right? I don't know why we'd bother/want to expose that to view-only users. 🤔

There's no harm in it, so I've added it, but not immediately sure why it's needed. But it'll probably simplify the code base when running selectors, so fair enough.

on_change is perfect, thanks, I just couldn't find that! 👍🏻

[techanvil]

@techanvil The main use of adSenseLinkedLastSyncedAt is knowing when the last sync was to see if we should do another sync, right? I don't know why we'd bother/want to expose that to view-only users. 🤔

There's no harm in it, so I've added it, but not immediately sure why it's needed. But it'll probably simplify the code base when running selectors, so fair enough.

Hi @tofumatt, thanks for pointing that out. Tbh I hadn't considered that properly either - not one of my finest reviews, I must say. Glad I could at least contribute something useful with the pointer to on_change().

I think you were right the first time around - if the setting isn't needed by the view-only user, we shouldn't need to include it in get_view_only_keys(). Sorry to go around in circles, but if you prefer to omit it after all, I'd be happy for us to take that approach. It's probably better to be precise about which settings do need to be exposed rather than include this one to make our selectors a bit easier to manage.

[tofumatt]

No worries! Sounds good, I've removed it from the IB 👍🏻

[techanvil]

Thanks @tofumatt! IB LGTM. :white_check_mark:

[nfmohit]

Hi folks! 👋

I have been browsing through the design doc and other issues for this epic and found out that the "AdSense linked" setting is referenced in both ways - adsenseLinked (e.g. in this issue) and adSenseLinked (e.g. in the ACs for #8049).

I can see in the design doc that it is referenced as adSenseLinked.

Which is the correct one to implement, keeping in mind that adsenseLinked is a pre-existing setting in Analytics 4 and it may have a pre-existing value?

Should we go forward with adsenseLinked? If so, I think the design doc and other issues should be updated for clarification. If we go with adsenseLinked, shouldn't we use adsenseLinkedLastSyncedAt (instead of adSenseLinkedLastSyncedAt) for consistency?

Or, should we remove the old adsenseLinked setting and start fresh with adSenseLinked? In my opinion, this would be cleaner if we didn't have any dependency on the older option.

CC: @tofumatt @kuasha420 @techanvil

Edit: @tofumatt, I've co-assigned you as the AC author.

[tofumatt]

Ah, so I originally wrote adSenseLinked, but then noticed we already had the value saved as adsenseLinked. I thought it would be easier to keep it, but if it's synced with UA, that might actually not be great, you're right. 😓

It seemed annoying to bother changing a bunch of things around just for that casing change, but the adsenseLinked in the GA4 is just a synced setting, right? It comes from UA, and just because a UA property is linked doesn't mean the GA4 one is 🤔

So I guess we should add the new setting and remove the old one. If that sounds good I'll audit all of the issues and make sure things are okay. There wasn't a lot of documentation around the existing adsenseLinked setting so I wasn't sure 🤔

[nfmohit]

Ah, so I originally wrote adSenseLinked, but then noticed we already had the value saved as adsenseLinked. I thought it would be easier to keep it, but if it's synced with UA, that might actually not be great, you're right. 😓

It seemed annoying to bother changing a bunch of things around just for that casing change, but the adsenseLinked in the GA4 is just a synced setting, right? It comes from UA, and just because a UA property is linked doesn't mean the GA4 one is 🤔

So I guess we should add the new setting and remove the old one. If that sounds good I'll audit all of the issues and make sure things are okay. There wasn't a lot of documentation around the existing adsenseLinked setting so I wasn't sure 🤔

You're right. It wouldn't make sense to carry over the legacy adsenseLinked setting. I'll remove that as part of this issue and add adSenseLinked.

Thank you!

[nfmohit]

@tofumatt One more question: Is it mandatory to use null as the default value for adSenseLinkedLastSyncedAt?

• Whenever the user's AdSense account or GA4 property changes, or when they disconnect AdSense module, we should reset both values to their original/default values.

As part of addressing this AC, I found out that our Module_Settings::merge method doesn't actually support null as a module setting value. See:

https://github.com/google/site-kit-wp/blob/6c316f535e3f31cdf913218e42df1a08f204c971/includes/Core/Modules/Module_Settings.php#L45-L56

When we go on to set it to null, it doesn't work. Does it need to be null by design, or is it okay to use 0 similar to googleTagLastSyncedAtMs?

[tofumatt]

Oh, I see. I don't think null is a requirement, it was just a bit nicer as it made clearer that there was never a sync for the current account. 0 is functionally the same, it's just a bit less explicit.

But it's totally fine to use. I'll update any issue where I said null 😄

[nfmohit]

Thank you!

[mohitwp]

QA Update ⚠️

• Tested on dev environment.
• Verified all conditions mentioned under QAB and couple of scenarios related to Dashboard sharing.
• Verified that on all conditions getting the expected result for snippet.

@nfmohit - In QAB, 7th steps mentioned that - "Using Site Kit settings, change the AdSense account". But, there is no option available under settings to changes AdSense A/c. Can you please verify if this step under QAB is correct ?

  **1) Set up Site Kit with the Analytics & AdSense modules.** https://github.com/google/site-kit-wp/assets/94359491/b046c100-7bbc-4906-ba39-146572904c9d **2) using the Site Kit settings, change the Analytics property.** https://github.com/google/site-kit-wp/assets/94359491/0adfe416-6908-444a-a26f-87e35c6e305e **3) Using Site Kit settings, disconnect the AdSense module. https://github.com/google/site-kit-wp/assets/94359491/17336968-7c0d-4eac-ad42-17084065321c 4) Share Analytics Module with another user, and login with that user** https://github.com/google/site-kit-wp/assets/94359491/5bffb267-07db-44a4-967d-01d779b2cab0

[nfmohit]

Thank you for pointing that out, @mohitwp. I've updated this section of the QAB for you. Thank you!

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified all conditions mentioned under QAB and couple of scenarios related to Dashboard sharing.
• Verified that on all conditions getting the expected result for snippet.

  **1) Set up Site Kit with the Analytics & AdSense modules.** https://github.com/google/site-kit-wp/assets/94359491/b046c100-7bbc-4906-ba39-146572904c9d **2) using the Site Kit settings, change the Analytics property.** https://github.com/google/site-kit-wp/assets/94359491/0adfe416-6908-444a-a26f-87e35c6e305e **3) change the AdSense account ID using snippets.** https://github.com/google/site-kit-wp/assets/94359491/8fd478d6-3bbf-47a5-8f9d-ba1a66b1dfb3 **4) Using Site Kit settings, disconnect the AdSense module. https://github.com/google/site-kit-wp/assets/94359491/17336968-7c0d-4eac-ad42-17084065321c **5) Share Analytics Module with another user, and login with that user** https://github.com/google/site-kit-wp/assets/94359491/5bffb267-07db-44a4-967d-01d779b2cab0

[aaemnnosttv]

These settings should both be made available in the view-only context (eg. to users using Dashboard Sharing).

Just noting here (which is consistent with the implementation) that only the setting for the linked state should be shared in a view-only context. The synced at timestamp wouldn't be relevant since view-only users can't sync the state. Everything looks good in the PR though 👍