Handle changing the connected Analytics property

[techanvil]

Feature Description

Ensure the Audience Segmentation feature is effectively reset when changing the connected Analytics property.

Additionally, ensure the sellected audiences are cleared when disconnecting Analytics.

See Analytics properties and selected audiences in the design doc.

---

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

Acceptance criteria

Implementation Brief

• [ ]

Test Coverage

QA Brief

Changelog entry

[techanvil]

I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one

[techanvil]

The audience caching aspect of the design doc has been sufficiently finalised, and I've moved this back to AC.

[benbowler]

I considered a few different approaches for this including creating a new REST endpoint, however the hooks approach seems to be to be the cleanest as it doesn't require adding Analytics specific logic to shared components like the module store or settings Footer component.

[eclarke1]

@benbowler should this one be in IBR?

[benbowler]

@eclarke1 not quite, I discussed the IB here with @aaemnnosttv last thing on Friday. I need to review it and may make some updates before putting it to IBR.

[benbowler]

My discussion with @aaemnnosttv revolved around changing the dismissed item/prompt slug, adding the account and property ID so that they would naturally be invalidated by switching the account/property.

The availableAudiences and availableAudiencesLastSyncedAt would still need to be reset as in my original IB.

Starting a thread here with @techanvil to get your thoughts before I update the IB.

[techanvil]

Hi @benbowler, this would be a good solution if we did want to retain the state when the user reverts to the same property later.

However, this is not actually the case. If the user reverts to the same property, we still want to re-show the Setup CTA Banner in order to allow them to re-setup the feature, and correspondingly show the setup success notifications once they have set it up again. We also want to re-show the Audience Creation Notice to give them another opportunity to create the SK audiences, and of course don't want to restore the "temporarily hidden" state of audiences as this would be confusing having set the feature up again.

So, we still want to delete the dismissed items/prompts when switching/resetting the property, and AFAICT it wouldn't really help to prefix the keys as suggested. Definitely a good pattern to bear in mind for other situations but I don't think it would be applicable here.

[techanvil]

Hey @benbowler, thanks for drafting this IB. A few points to consider:

• [ ] Check the user has setup permissions before continuing current_user_can( Permissions::SETUP );.

Hmm... This might be a bit of a redundant check as by the time reset_audience_settings() is called, we should be confident the user has the correct permissions. If we definitely need it let's keep it but otherwise it would be nice to drop this point.

• [ ] Get all users using get_authenticated_users, loop through these users and for each user, using the switch_user method to switch the user context (eg here):

Non-authenticated users can also "temporarily hide" tiles and have their own audience settings, so we need a different approach to iterating the users for at least these values.

Maybe, instead of reusing the get_authenticated_users() logic, we can invoke get_users() to only return users who have the googlesitekit_audience_settings user option set. This should suffice to retrieve all the users we're interested in, because all users who have set up and interacted with the Audience Segmentation feature will have set their googlesitekit_audience_settings as part of the setup process.

get_users(
   array(
      'meta_key'     => $this->user_options->get_meta_key( 'googlesitekit_audience_settings' ),
      'meta_compare' => 'EXISTS',
      'fields'       => 'ID',
   )
);

• [ ] Call Dismissed_Items->remove for the slugs audience-segmentation-add-group-notice, audience_segmentation_setup_success_notification and audienceSegmentationIntroductoryOverlayNotification.

We don't want to delete the audienceSegmentationIntroductoryOverlayNotification slug, as this is for the Introductory Popup which is (deliberately) not mentioned in the AC. However we do want to delete the slug for the Setup Success Settings Notice which is being introduced in the dependency https://github.com/google/site-kit-wp/issues/8178.

• [ ] Get the analytics module settings using $this->get_settings(), remove the key availableAudiences and availableAudiencesLastSyncedAt, and update the module settings using $module->get_settings()->merge( (array) $request['data'] );.

The snippet doesn't look quite right here - aside from $request['data'] not being relevant, the call to merge() doesn't seem correct for values we want to delete. That said - rather than delete them we can reset them to their default values of null and 0, in which case merge() will be useful.

• [ ] Do a new action in includes/Core/Modules/Modules.php, deactivate_module called "googlesitekit-deactivate-module-" . $slug.
  • [ ] Update includes/Modules/Analytics_4.php, use add_filter to call the new reset_audience_settings method on the googlesitekit-deactivate-module-analytics-4 hook.

We don't need a new action for this, we have the Analytics_4::on_deactivation() method that we can amend.

• [ ] Update the "googlesitekit_pre_save_settings_" . $slug action call to pass (array) $request['data'] as the first param.
  • [ ] Update includes/Modules/Analytics_4.php, create a new method reset_audience_settings_on_account_or_property_change which take one param $new_settings.
    • [ ] Get the current analytics settings using $this->get_settings.
    • [ ] If either the propertyID or accountID has changed, call the reset_audience_settings method.
  • [ ] Update includes/Modules/Analytics_4.php, use add_filter to call the new reset_audience_settings_on_account_or_property_change method on the "googlesitekit_pre_save_settings_" . $slug hook.

Rather than using the googlesitekit_pre_save_settings_ hook, we can add to the block of code in the Analytics settings' on_change() handler which runs when the property ID changes (this should be fine for the requirements here, we don't really need to look for the account ID changing as well).

[techanvil]

Update to the above:

Maybe, instead of reusing the get_authenticated_users() logic, we can invoke get_users() to only return users who have the googlesitekit_audience_settings user option set. This should suffice to retrieve all the users we're interested in, because all users who have set up and interacted with the Audience Segmentation feature will have set their googlesitekit_audience_settings as part of the setup process.

Upon reflection, I've realised this wouldn't give us all users we are interested in - users who have simply dismissed the initial setup banner, via the audience_segmentation_setup_cta-notification prompt, obviously won't have set the feature up and so won't have the googlesitekit_audience_settings entry in the usermeta table. We'd still need to iterate over the authenticated users (or, maybe an alternative approach e.g. finding all users who have dismissed prompts).

[ivonac4]

Leaving a note from the AS sync: Tom will speak with Evan about Ben's suggestions about this task, since it is touching the fundamental parts of the project.

[techanvil]

@aaemnnosttv and I had a good chat about this but haven't yet determined the way to go here - he's going to take another look async and leave his thoughts on the matter.

[benbowler]

Thanks @techanvil, I'll add @aaemnnosttv to the assignments so that it appears on his board when filtered by assignment.

[aaemnnosttv]

Thanks for your patience guys!

• When the connected Analytics property is changed, either when selecting a new property via the Analytics settings, when disconnecting Analytics, or resetting Site Kit
• The dismissal state for the following items should be cleared for all users:

I'm not sure we need to specify all the ways it could change, but the inclusion of the reset here is interesting of course because dismissed items are not included in this (reset) currently, but perhaps they should? I.e. maybe dismissed items shouldn't be persistent? Maybe we could have some which are and others which aren't, in which case we'd need multiple options, but that could be okay. The reason I mention it is because it applies to all other DI's not just those for AS, in which case it might be better addressed in a separate issue if we go down that route, although there is additional work in progress to potentially consolidate our various dismissible entities into fewer so we should probably hold on making substantial changes until that path is clarified.

• The Setup CTA Banner.

This makes sense to be a non-permanent dismissal.

• The Audience Creation Notice in the Selection Panel.

This makes sense to be a non-permanent dismissal as well. @techanvil and I also discussed how there is no other way to trigger this currently which should actually make it non-dismissible I think since this is important functionality of the feature.

• The Setup Success Notice displayed when having set the feature up on the dashboard.
• The Setup Success Settings Notice displayed when having set the feature up on the Settings screen.

Both of these don't actually need dismissals associated with them since they are only triggered by URL parameters that should only be added in response to these actions.

• The "temporarily hidden" state for any hidden audiences should be cleared.
• The audience selection should be cleared for all users.

These are really a kind of module-specific user preference, so this might be worth exploring as part of https://github.com/google/site-kit-wp/issues/8810

• The list of cached audiences should be cleared.

This is perhaps the simplest to address since it is stored in module settings, so it could be filtered out, similar to how the ownerID is set when owned settings change. It would automatically be cleared on module disconnect or reset as well.

---

TL;DR some of these are simple changes, where others probably warrant more significant architectural changes which are out of scope here.

One more idea that comes to mind here as we reconsider the way we think about dismissals would be to potentially dismiss certain things with "layers". That is, we might have a generic dismissal as a temporary or non-permanent stored item, and another more specific dismissal (e.g. specific to a given GA property) be permanent.

Tactically speaking for now, I think it could be okay to query all users with SK dismissed items (not necessarily currently authenticated) to clear these out, but we should only query users by a single meta key, otherwise we need to resort to a manual query for performance reasons. See https://github.com/google/site-kit-wp/issues/2870#issuecomment-816441730 We could cap the limit at some reasonable number and loop over them to clear out things as defined here where needed.

[techanvil]

Thanks for your feedback, @aaemnnosttv!

• When the connected Analytics property is changed, either when selecting a new property via the Analytics settings, when disconnecting Analytics, or resetting Site Kit
• The dismissal state for the following items should be cleared for all users:

I'm not sure we need to specify all the ways it could change, but the inclusion of the reset here is interesting of course because dismissed items are not included in this (reset) currently, but perhaps they should? I.e. maybe dismissed items shouldn't be persistent? Maybe we could have some which are and others which aren't, in which case we'd need multiple options, but that could be okay. The reason I mention it is because it applies to all other DI's not just those for AS, in which case it might be better addressed in a separate issue if we go down that route, although there is additional work in progress to potentially consolidate our various dismissible entities into fewer so we should probably hold on making substantial changes until that path is clarified.

Hmm... that's a good point about the inclusion of the reset. Actually, it's made me rethink that aspect and I've realised we don't really need to clear the dismissed entities on reset after all.

Well, maybe we should actually clear all dismissed entities, or take a layered approach as per your later comment, but that's a separate conversation and one we should indeed consider as part of, or after the work to potentially consolidate the entities (cc @nfmohit).

Anyhow - back to my point about not needing to clear these items on reset; seeing as we need to connect Analytics in order to make the Audience Segmentation feature available, when we connect the GA4 property after a reset, the entities will be cleared in the settings on_change() handler in response to the property ID changing at that point.

Arguably, we don't need to clear them on disconnecting Analytics either because the same logic will apply in this scenario when Analytics is reconnected.

I have therefore gone ahead and removed the requirements relating to resetting and disconnecting from the AC, and updated the IB accordingly.

• The Setup Success Notice displayed when having set the feature up on the dashboard.
• The Setup Success Settings Notice displayed when having set the feature up on the Settings screen.

Both of these don't actually need dismissals associated with them since they are only triggered by URL parameters that should only be added in response to these actions.

As discussed on a recent call, these notices do currently use the dismissed items API to manage their dismissal state; they don't need to, and I've created https://github.com/google/site-kit-wp/issues/9107 to address this.

I don't envisage that we'll want to use query parameters to manage the state though - typically we use query params to convey state when landing on a page, but both of these setup success scenarios are fully contained within the lifecycle of a single page load. So, I've specced that these notices don't need to persist upon reloading the page as they do now due to the use of dismissible items.

Deleting a couple of extra dismissed items will be quicker than the rewrite to use client-side state instead of the dismissed items, so I have kept the deletion of these items in this issue and specced #9107 as a P2 for post-release.

• The "temporarily hidden" state for any hidden audiences should be cleared.
• The audience selection should be cleared for all users.

These are really a kind of module-specific user preference, so this might be worth exploring as part of #8810

The audience selection is already included in #8810, it's the configuredAudiences setting.

I don't think it's worth spending additional time refactoring the "temporarily hidden" state in #8810, this is non-essential and would be better pursued post-launch if at all, IMHO.

• The list of cached audiences should be cleared.

This is perhaps the simplest to address since it is stored in module settings, so it could be filtered out, similar to how the ownerID is set when owned settings change. It would automatically be cleared on module disconnect or reset as well.

This is indeed straightforward and we already have an entry point to add the additional field to, which I've linked to in the IB.

Tactically speaking for now, I think it could be okay to query all users with SK dismissed items (not necessarily currently authenticated) to clear these out, but we should only query users by a single meta key, otherwise we need to resort to a manual query for performance reasons. See #2870 (comment) We could cap the limit at some reasonable number and loop over them to clear out things as defined here where needed.

As there are both dismissed items and prompts to consider, we'll need to query by two meta keys. I have therefore specced in the IB that we should use a custom query to retrieve the user IDs, taking the above linked discussion into account.

Please note I've simply specified SELECT DISTINCT user_id FROM usermeta rather than selecting from the users table with a sub-select to retrieve the user IDs from the usermeta table. It seems this should be sufficient but maybe we should do the sub-select approach to make sure the user IDs are indeed present in the users table? I've also specced an arbitrary LIMIT 100 to avoid an unbounded user iteration, although this does of course leave an unhandled edge case where the query returns over 100 users (highly unlikely in practice by the sound of it). Maybe we could address this edge case post-release.

Although we may continue this conversation I think the IB is ready to review, and as you've got the context on this I've assigned it to you in IBR. Please see what you think.

[aaemnnosttv]

Thanks @techanvil, that all sounds good to me. Two final thoughts here:

1. Looking over the AC again, I'm thinking maybe we shouldn't reset the hidden state of the area in response to a property change since that is more of a preference that isn't necessarily invalidated by changing the property. Also this is manageable via the settings so the user can always turn it back on themselves whereas dismissed items, etc cannot.
2. Given the rather large size of the module class, what do you think about adding this in its own class, similar to the plugin Reset instead?

[techanvil]

Thanks @aaemnnosttv, good points there.

1. I've updated the AC/IB to remove the requirement to clear the hidden state of the widget area.
2. I have amended the IB to spec a new Reset_Audiences class which encapsulates the reset behaviour. I was debating whether adding an additional settings change handler was the best approach, and in the end I've gone with that, but we could simply call reset_audience_data from the existing change handler in Analytics_4 instead. Interested to see what you think.

[techanvil]

Update: I've also added the hidden state for the No Audiences Banner and the didSetAudiences flag that we are introducing via https://github.com/google/site-kit-wp/issues/8577 to the list of settings to clear on reset.

[aaemnnosttv]

LGTM, thanks @techanvil 👍

IB ✅

[nfmohit]

@techanvil I've reviewed the PR for this issue and the current state LGTM! During my review, I found that a part of the IB and its consequent implementation is not correct, the following part specifically:

• In the register method, call Analytics_4/Settings -> on_change to set up a change handler.

  • [x] When the propertyID value changes, call Analytics_4/Settings -> merge to set the following values to their defaults:

    • [x] availableAudiences => null
    • [x] availableAudiencesLastSyncedAt => 0
    • [x] audienceSegmentationSetupComplete => false

• [x] Remove availableAudiencesLastSyncedAt from the existing call to reset settings in Analytics_4.

I've mentioned about this in detail here and have changed this in the PR to correct it.

In summary, to reset the availableAudiences and audienceSegmentationSetupCompletedBy to their default value of null, we do need to use the pre_update_option_googlesitekit_analytics-4_settings hook as the Module_Settings::merge method (used in the on_change handler) does not support null as a valid setting value.

In the spirit of DRY, I've not repeated the on_change handler and the pre_update_option_googlesitekit_analytics-4_settings hook in the Reset_Audiences class, but rather left the reset of the AS-specific module settings within Analytics_4 with appropriate test coverage. Let me know if you disagree and if these should be recreated in Reset_Audiences.

As I have made some significant changes to the PR, I'm assigning this issue to you for MR. Thanks!

CC: @benbowler

[kelvinballoo]

QA Update ⚠️

Hi @benbowler on the outset, this is working as expected. I have a couple of clarifications to be confirmed though, as listed below:

ITEM 1: ⚠️ I have used 3 users to the site. When testiing for the googlesitekitpersistent_dismissed_items, I dismissed all the required items and changed the Analytics property. It seems that not all of them is 100% cleared. I Refer to the details where:

• For ID 35, it was cleared.
• For the other IDs 72 and 90, the "audienceSegmentationIntroductoryOverlayNotification" remained. I assume that's fine because it's not part of the items we worked on. Can you confirm?

---

ITEM 2: ⚠️ The AC references the following: The "temporarily hidden" state for any hidden audiences should be cleared. The "temporarily hidden" state for the No Audiences Banner should be cleared.

Is there any specific properties that I should check for temporarily hidden items? Or this is part of the configuredAudiences?

---

ITEM 3: ⚠️ For iwpc986_googlesitekit_audience_settings, whenever we disconnect or change analytics properly, the values would not only be cleared but the entired rows will be deleted. I assume that's expected. Could you help to confirm?

---

ITEM 4: ⚠️ For the WP Options table, I was expecting the following after disconnecting/changing analytics: availableAudiences => null availableAudiencesLastSyncedAt => 0 audienceSegmentationSetupComplete => false

The actual results were as follows: "availableAudiences";N;s:30:"availableAudiencesLastSyncedAt";i:0;s:36:"audienceSegmentationSetupCompletedBy";N;s:14:"detectedEvents";a:0:{}}

• For availableAudiences, I assume N is synonymous to null
• For availableAudiencesLastSyncedAt, it became 0 as expected.
• For audienceSegmentationSetupCompletedBy, I was expecting false but it was 'N'. Is that expected?

---

Other than that, iwpc986_googlesitekitpersistent_dismissed_prompts were reset successfully

[techanvil]

Thanks @kelvinballoo, I've taken a look at your feedback.

ITEM 1: ⚠️ I have used 3 users to the site. When testiing for the googlesitekitpersistent_dismissed_items, I dismissed all the required items and changed the Analytics property. It seems that not all of them is 100% cleared. I Refer to the details where:

• For ID 35, it was cleared.
• For the other IDs 72 and 90, the "audienceSegmentationIntroductoryOverlayNotification" remained. I assume that's fine because it's not part of the items we worked on. Can you confirm?

That's correct, we have deliberately excluded this key from the reset logic. Once a secondary user has seen the introductory popup/banner for the feature, they have been introduced to it so don't need to see it again. We might want to think about this again during the bug bash but it's the specced behaviour for now at least.

ITEM 2: ⚠️ The AC references the following: The "temporarily hidden" state for any hidden audiences should be cleared. The "temporarily hidden" state for the No Audiences Banner should be cleared.

Is there any specific properties that I should check for temporarily hidden items? Or this is part of the configuredAudiences?

You can verify this via the plugin UI by temporarily hiding the items, changing the property, then reconnecting the previous property and confirming that the items are no longer hidden.

If you want to check it in the DB, you should check the googlesitekitpersistent_dismissed_items entry for the key audience-segmentation-no-audiences-banner for the No Audiences Banner, and any key starting with audience-tile for the hidden audiences.

ITEM 3: ⚠️ For iwpc986_googlesitekit_audience_settings, whenever we disconnect or change analytics properly, the values would not only be cleared but the entired rows will be deleted. I assume that's expected. Could you help to confirm?

Thanks for flagging this. It's actually not as expected and required a followup to fix it, because we shouldn't be completely clearing the settings - we want to retain the user's choice for the isAudienceSegmentationWidgetHidden as per the AC/IB update mentioned here. So we should actually be calling merge() on the Audience_Settings instance to set configuredAudiences and didSetAudiences back to their defaults as per the IB rather than deleting the settings.

Reviewing Audience_Settings I can see it will need a bit of additional work to allow merging null for configuredAudiences, which wasn't allowed for in the IB, so I feel we should mark this as a known issue and address it in a followup issue rather than a followup PR for this one. Please can you create an issue in Triage and I'll add AC for it?

ITEM 4: ⚠️ For the WP Options table, I was expecting the following after disconnecting/changing analytics: availableAudiences => null availableAudiencesLastSyncedAt => 0 audienceSegmentationSetupComplete => false

The actual results were as follows: "availableAudiences";N;s:30:"availableAudiencesLastSyncedAt";i:0;s:36:"audienceSegmentationSetupCompletedBy";N;s:14:"detectedEvents";a:0:{}}

• For availableAudiences, I assume N is synonymous to null
• For availableAudiencesLastSyncedAt, it became 0 as expected.
• For audienceSegmentationSetupCompletedBy, I was expecting false but it was 'N'. Is that expected?

This is as expected, yes - the audienceSegmentationSetupCompleted setting was renamed to audienceSegmentationSetupCompletedBy with a default value of null after the IB was drafted so it's a little out of date.

Btw - you're correct with regard to the N => null mapping. It is possible to use the WP CLI on InstaWP to retrieve the mapped version of the data - it takes a little setting up, you need to go to https://app.instawp.io/commands, create a command which runs wp option get googlesitekit_analytics-4_settings (or whichever setting you want to query), then run it via the Commands menu item for a site:

[kelvinballoo]

QA Update ✅

Thanks @techanvil , noted on items 1, 2, 4 and 5. For item 3, I've raised an issue here: https://github.com/google/site-kit-wp/issues/9432 and assigned to you.

For ITEM 2, I've done a test as you suggested and the 'Temporarily Hidden' badge is not carried through after changing analytics property or disconnecting the Analytics module and adding back the Analytics property. YOu may refer below for the video.

• Also verified per the video, when we change analytics property, the tiles are reset and they don't appear anymore.
• Also verified that when we disconnect analytics property, the tiles are gone too.

https://github.com/user-attachments/assets/6d05b994-703a-4348-9e45-fd4f49fe5237

Other items were also verified good : ✅

• googlesitekitpersistent_dismissed_items and googlesitekitpersistent_dismissed_prompts were reset successfully when changing analytics property.

  

• For the WP Options table, the 3 parameters: AvailableAudiences, availableAudiencesLastSyncedAt and audienceSegmentationSetupCompletedBy were all reset successfully when changing analytics property or disconnecting it.

  Actual results were as follows in the DB editor table after reset: "availableAudiences";N;s:30:"availableAudiencesLastSyncedAt";i:0;s:36:"audienceSegmentationSetupCompletedBy";N;s:14:"detectedEvents";a:0:{}}

• This was tested on a couple of WP Users : 2 admins and one editor and results were consistent

Moving ticket to Approval.