Relocate the per user Audience Settings out of analytics module settings so it can be accessed when the module is disconnected

[kuasha420]

Feature Description

While working on #8156 it was discovered that the audience settings needs to be accessed when Analytics is disconnected. This issue will handle the refactor of the settings and enhancing the isActive requirement of the Widget introduced in the aforementioned issue.

More logical place for the settings is the user store, In the short term, we can move the selectors and rest endpoint to facilitate that. However as @aaemnnosttv pointed out in our private conversation, maybe it's time we introduced user preference infrastructure, so instead of having separate rest endpoint and store partial for Audience, Key metrics, user input etc, we can have a single class, rest endpoint and store partial for all of them. We can use WordPress filters to modify this from within module or other place when necessary and migrate the existing settings on the fly in future. Something to consider while working on the definition of the issue!

---

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

Acceptance criteria

• The Audience Settings (which comprises of configuredAudiences and isAudienceSegmentationWidgetHidden settings as of writing) should be moved from analytics module store to core/user store so the settings can be accessed regardless of Analytics module connection status.
  • This includes the Audience_Settings class, REST endpoints (GET:audience-settings and POST:audience-settings) and data store partial.
  • The routes for the REST endpoints should be updated to sit under the core/user/data path.
  • Ensure get GET route is still preloaded.
• The Connect Analytics CTA widget should only be shown if the Audience Segmentation feature has been set up for the current user and Analytics is disconnected.

Implementation Brief

• Create new namespace Google\Site_Kit\Core\User in includes/Core/User.
• Move the Google\Site_Kit\Modules\Analytics_4\Audience_Settings class to the newly created namespace. ie. Google\Site_Kit\Core\User\Audience_Settings.
• Move the GET:audience-settings and POST:audience-settings endpoints from Analytics module to a newly created REST controller. ie. includes/Core/User/REST_Audience_Settings_Controller.php
  • Ensure the GET endpoint is preloaded using googlesitekit_apifetch_preload_paths filter in the register method.
  • Ensure the permissions are correct. ie. current_user_can( Permissions::VIEW_DASHBOARD.
  • Ensure that the base path for the both REST routes is core/user/data.
• Create an entrypoint for the new namespace in includes/Core/User/User.php and register both settings and rest route here.
• Register the new namespace inside the init action callback in includes/Plugin.php::register().
• Move the assets/js/modules/analytics-4/datastore/audience-settings.js data store partial from modules/analytics-4 to the core/user store and update the usage of the selectors and actions across the codebase.
  • Update the fetchStores to use the updated REST endpoint.
• In assets/js/modules/analytics-4/index.js:
  • Update the isActive callback of audienceConnectAnalyticsCTA widget to only render when configuredAudiences is a non-empty array and isAudienceSegmentationWidgetHidden is false.

Test Coverage

• Add test coverage for the newly added REST controller.
• Update location of the tests of moved infrastructure and codes.
• Fix any failing tests.

QA Brief

• Setup Site Kit with Analytics module
• Do overall testing around Audience Segmentation epic
• Verify that everything still works as expected. You can also verify that saving/deselecting audience groups is reflected in the option - now migrated to the core user store await googlesitekit.data.select('core/user').getAudienceSettings(). It should output an object with groups inside an array

Changelog entry

• Detached audience segmentation settings from the Analytics module.

[techanvil]

Hi @kuasha420, thanks for drafting the AC.

My concern is that adding a common user preferences infrastructure is a bit of scope creep that we are trying to cut out for the development of the initial Audience Segmentation release.

I realise it sounds quite simple, but I feel that designing and implementing a generic subsystem like this is inherently going to be more complicated and time consuming than simply moving the current Audience Settings code out of the Analytics module to the core User area of the codebase,

What I'd suggest is that we take this more simple and specific approach for the initial iteration, but create a followup issue - which can be defined outside of the Audience Segmentation epic - to create the common infrastructure, and migrate the Audience, Key Metrics, User Input settings/preferences over to it.

WDYT? If so, please can you amend the AC and also create a followup issue to capture the above? (Just one issue will do for now although we could split it out to multiple issues later).

[kuasha420]

@techanvil Thanks for the review and raising this. Yes, I do understand the concern here. However, in the defense of doing the common infra now, I'd like to point one thing.

If we just move the settings to core user area, we will eventually need to create a migration/migrate on the fly the audience segmentation related settings. This will be the case for already launched features like User Input and Key Metrics, of course. But for audience segmentation, this can be prevented.

Also, the User Preference is going to be for simple Key-Value pairs, so I personally don't think it is going to be too difficult compared to just moving the Audience settings, except renaming the keys.

For the above reason, if we can do this right now without a vast investment, (ie. multiple level bump on the estimate), I'd put this forward to you once more for consideration. 🙏

If you don't agree, feel free to send it back my way, and I'll update the ACs and create follow up issue as suggested!

Cheers. 🥂

[techanvil]

Thanks @kuasha420, I appreciate your point, it's a fair one and it would indeed be nice to avoid having to migrate the Audience Segmentation settings.

However - with the epic running hot as it is, we are in a position where we really want to minimise any work that is not essential to the delivery of the feature. Also - although it doesn't sound too difficult, I do think it's going to be, within relative terms, significantly more work to implement the new infrastructure compared to simply moving what we've already got.

We'd need to introduce a multi-option version of the User_Setting class (or something along those lines), a new datastore, cover all of this with tests and indeed test it thoroughly to ensure it works for multiple user options. Plus do a smoke test of the existing Audience Segmentation feature to make sure it still works as expected.

This feels like more than a single bump on the estimate compared to mostly moving existing code and performing a smoke test (obviously, in either case we'll also need to also the last point in the AC so this doesn't factor in the comparison). We should also be conscious that things that look simple often have a tendency to be less so than we thought 😅.

We'll still need to introduce a new REST controller, but maybe we could even add this in a forward-looking way so that it can be extended later to cover additional settings.

Going back to the point about the settings migration - while it would of course be nice to avoid needing to migrate the Audience Segmentation settings, seeing as we'll have to migrate Key Metrics and User Input anyway, we'll already have the pattern established so migrating AS settings as well should be relatively low impact compared to if we had to migrate the AS settings in isolation. Something else to bear in mind - although we can't bank on it, say if Team S are running a bit low on work they might even be able to pick the infra work up before AS is released and we could potentially avoid the migration anyway.

Sooo, with all this in mind, I do still feel it would be preferable to reduce the scope of this issue and address the rest later. Unless you have any further considerations, please do ahead with that approach. Thanks! 🍻

[kuasha420]

Sounds reasonable @techanvil. I've updated the AC for you to review. I'll file a follow up issue about exploring user preferences as you've suggested. Cheers.

[techanvil]

Thanks @kuasha420. That generally LGTM - I have made a couple of minor tweaks to the AC - adding a added a point to clarify the new paths and rewording the final point slightly to make it a bit more user-oriented.

AC ✅

[eugene-manuilov]

Sending this ticket back to execution. Left one comment to @zutigrm and one comment to @kuasha420 and @techanvil.

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified audience segmentation set up.
• Verified audience segmentation selection and deselection of group.
• Done smoke testing of audience segmentation functionality.
• All functionality are same as latest environment.
• Verified await googlesitekit.data.select('core/user').getAudienceSettings() output an object with groups inside an array

[aaemnnosttv]

@kuasha420 @techanvil it sounds like this one took a simpler approach for the sake of time at the moment which makes sense. I still think it's worth moving towards a generic API for preferences where this could live rather than replicating this approach for other situations. Do we have an issue to pick this up as a follow-up outside of the epic?

[techanvil]

Hey @aaemnnosttv, I've created #9325 an an initial entry point for following up on this. We'll probably want to create more issues to backport existing user settings, but we can figure that out as we get further into the definition of the initial issue.