Remove GET:audiences endpoint and getAudiences selector #8606

ankitrox · 2024-04-23T14:36:12Z

Summary

Addresses issue:

Remove the GET:audiences endpoint and the getAudiences() selector, replacing usage of this with getAvailableAudiences(). #8487

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…point.

github-actions · 2024-04-24T06:11:41Z

Build files for e68de59 have been deleted.

github-actions · 2024-04-24T06:29:57Z

Size Change: +40 B (0%)

Total Size: 1.43 MB

Filename	Size	Change
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	53.7 kB	+12 B (0%)
`./dist/assets/js/googlesitekit-activation-********************.js`	23.8 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	57.4 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.2 kB	+4 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.1 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	19.1 kB	+10 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	24.8 kB	-5 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	95.7 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.7 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	111 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	109 kB	-36 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.4 kB	+54 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	58 kB	+2 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	30 kB	+5 B (0%)
`./dist/assets/js/googlesitekit-settings-********************.js`	59.7 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-splash-********************.js`	71.7 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	46.9 kB	-5 B (0%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	33.4 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	60.2 kB	+2 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.2 kB
`./dist/assets/css/googlesitekit-authorize-application-css-********************.min.css`	711 B
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.48 kB
`./dist/assets/js/29-********************.js`	2.8 kB
`./dist/assets/js/30-********************.js`	2.28 kB
`./dist/assets/js/31-********************.js`	3.72 kB
`./dist/assets/js/32-********************.js`	929 B
`./dist/assets/js/33-********************.js`	888 B
`./dist/assets/js/34-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	778 B
`./dist/assets/js/googlesitekit-api-********************.js`	10.1 kB
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.88 kB
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10 kB
`./dist/assets/js/googlesitekit-data-********************.js`	2.18 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	10.1 kB
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	71.2 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	19.6 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B
`./dist/assets/js/googlesitekit-vendor-********************.js`	316 kB
`./dist/assets/js/runtime-********************.js`	1.29 kB

_{compressed-size-action}

techanvil · 2024-04-24T11:49:35Z

...ts/js/modules/analytics-4/components/dashboard/AudienceSegmentation/AudienceTiles.stories.js

@@ -124,7 +124,7 @@ export default {

 				registry
 					.dispatch( MODULES_ANALYTICS_4 )
-					.receiveGetAudiences( { audiences: audiencesFixture } );
+					.setAvailableAudiences( { audiences: audiencesFixture } );


This should provide an array of objects that represent the cached audiences, rather than the underlying fixtures.

I.e. they should have the shape as described in the AC for #8486 (with the exception of the "All Users" displayName which should be mapped to "All visitors" as part of this issue).

Suggested change

.setAvailableAudiences( { audiences: audiencesFixture } );

.setAvailableAudiences( mappedCachedAudiences );

Actually, further note that these calls to setAvailableAudiences() and setConfiguredAudiences() should be replacable by a single call to receiveGetSettings():

registry .dispatch( MODULES_ANALYTICS_4 ) .receieveGetSettings( { availableAudiences, configuredAudiences } );

techanvil · 2024-04-24T11:53:51Z

includes/Modules/Analytics_4.php

 				$audience_item = array(
 					'name'        => $audience->getName(),
-					'displayName' => $audience->getDisplayName(),
+					'displayName' => ( 'All Users' === $display_name ) ? __( 'All Visitors', 'google-site-kit' ) : $display_name,


Please note the value for "All visitors" should have a lower-case initialled "visitors", and it should also not be translated (the value it's replacing is not translated and is always created in English):

Suggested change

'displayName' => ( 'All Users' === $display_name ) ? __( 'All Visitors', 'google-site-kit' ) : $display_name,

'displayName' => ( 'All Users' === $display_name ) ? 'All visitors' : $display_name,

techanvil · 2024-04-24T11:55:13Z

assets/js/modules/analytics-4/components/dashboard/AudienceSegmentation/AudienceTiles.js

+		const audienceData =
+			select( MODULES_ANALYTICS_4 ).getAvailableAudiences();
+
+		return Object.values( audienceData );


This call to Object.value() should no longer be needed:

Suggested change

const audienceData =

select( MODULES_ANALYTICS_4 ).getAvailableAudiences();

return Object.values( audienceData );

return select( MODULES_ANALYTICS_4 ).getAvailableAudiences();

…mentation/AudienceTiles.stories.js Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

…mentation/AudienceTiles.js Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

techanvil · 2024-04-24T12:06:08Z

...ts/js/modules/analytics-4/components/dashboard/AudienceSegmentation/AudienceTiles.stories.js

@@ -124,7 +124,7 @@ export default {

 				registry
 					.dispatch( MODULES_ANALYTICS_4 )
-					.receiveGetAudiences( { audiences: audiencesFixture } );
+					.setAvailableAudiences( mappedCachedAudiences );


@ankitrox sorry if this suggestion was a bit unclear - it was for illustrative purposes, you'll still need to create mappedCachedAudiences (feel free to use a different name for it too).

@techanvil Thank you. I spotted this that I committed it by mistake, I changed it to audiencesFixture which should work.

Thanks, @ankitrox - but, this still needs an update. As per my previous comment, the audience fixtures represent the audiences as returned from the GA4 API.

This is not representative of the mapped versions of the audience objects that will be cached and therefore stored in the availableAudiences setting.

For reference here's the audiences fixture, as you can see it contains a set of Audience resources which have the following shape:

{ "name": string, "displayName": string, "description": string, "membershipDurationDays": integer, "adsPersonalizationEnabled": boolean, "eventTrigger": { object ([AudienceEventTrigger](https://developers.google.com/analytics/devguides/config/admin/v1/rest/v1alpha/properties.audiences#AudienceEventTrigger)) }, "exclusionDurationMode": enum ([AudienceExclusionDurationMode](https://developers.google.com/analytics/devguides/config/admin/v1/rest/v1alpha/properties.audiences#AudienceExclusionDurationMode)), "filterClauses": [ { object ([AudienceFilterClause](https://developers.google.com/analytics/devguides/config/admin/v1/rest/v1alpha/properties.audiences#AudienceFilterClause)) } ] }

As discussed in #8486, these should be mapped to a set of objects with the following shape which are then cached in the availableAudiences setting:

{ "name": string, "displayName": string, "description": string, "audienceType": string, // `"DEFAULT_AUDIENCE"`, `"SITE_KIT_AUDIENCE"` or `"USER_AUDIENCE"`. "audienceSlug"`: string, // `"all-users"`, `"purchasers"`, `"new-visitors"`, `"returning-visitors"`, or an empty string. }

So, the audiences fixture needs mapping to objects with the above shape to pass to setAvailableAudiences(). It might be worth creating a new fixture, say available-audiences.json, rather than mapping these in code for the tests etc.

Add support of audienceSlug to tooltip component.

techanvil · 2024-04-25T16:04:10Z

assets/js/modules/analytics-4/datastore/__fixtures__/available-audiences.json

+        "description": "Description",
+        "displayName": "Test Audience",
+        "audienceType": "USER_AUDIENCE",
+        "audienceSlug": "test-users"


Hey @ankitrox just a quick heads up - please note the audienceSlug is set to an empty string for user-defined audiences, as can be seen here:

site-kit-wp/includes/Modules/Analytics_4.php

Lines 2244 to 2245 in 109d750

// Return an empty string for user defined audiences.

return '';

Suggested change

"audienceSlug": "test-users"

"audienceSlug": ""

techanvil · 2024-04-25T16:16:05Z

assets/js/modules/analytics-4/datastore/__fixtures__/available-audiences.json

+    {
+        "name": "properties/12345/audiences/1",
+        "description": "All users",
+        "displayName": "All Users",


@ankitrox, also the displayName here should be "All visitors" to reflect how it will be mapped as per the related change in this PR:

Suggested change

"displayName": "All Users",

"displayName": "All visitors",

techanvil

Hi @ankitrox, nice work here so far.

I've left a few comments, please take a look.

Also, the QA Brief will need an update as it's currently focused on the change to createAudience() which will be removed. This should instead reflect the change to getAvailableAudiences() (i.e. the sync should be triggered when the value is initially null), and it's also worth including a QAB point to give the AudienceTiles component a quick check in Storybook to verify it's unchanged.

techanvil · 2024-04-26T11:22:10Z

assets/js/modules/analytics-4/datastore/audiences.js

+			// Sync available audiences in the state.
+			if ( error === undefined ) {
+				yield fetchSyncAvailableAudiencesStore.actions.fetchSyncAvailableAudiences();
+			}
+


Hi @ankitrox. Sorry I didn't spot this prior to implementation - I realise this is specified in the IB, but we shouldn't really be syncing cached audiences in createAudience(). When we initially set the feature up, we'll be creating the "new visitors" and "returning visitors" audiences via two calls to createAudience(). It would be preferable to avoid two calls to sync the audiences at the point, rather we can dispatch the syncAvailableAudiences() action once both createAudience() calls have completed.

So, please can you remove this addition?

Suggested change

// Sync available audiences in the state.

if ( error === undefined ) {

yield fetchSyncAvailableAudiencesStore.actions.fetchSyncAvailableAudiences();

}

Cc @kuasha420 @nfmohit

techanvil · 2024-04-26T11:28:33Z

assets/js/modules/analytics-4/datastore/audiences.js


+		// If available audiences not present, sync the audience in state.
 		if ( audiences === undefined ) {


As per the IB, this condition should explicitly test for audiences being null, rather than undefined.

We generally use undefined to indicate that a given entity hasn't loaded yet (i.e. it's not been retrieved from the backend).

So, when audiences was being retrieved via its own API call, this test for undefined made sense. However, now audiences is retrieved from the availableAudiences setting, which means the loading is handled for us via the underlying Analytics settings store.

What we want to do is to ensure that the audience cache is synced when availableAudiences is loaded and determined to be in its default state, i.e. it's null:

site-kit-wp/includes/Modules/Analytics_4/Settings.php

Line 103 in 95b40cb

'availableAudiences' => null,

Therefore, please can you update this condition as follows, and please also add a bit of test coverage for this aspect:

Suggested change

if ( audiences === undefined ) {

if ( audiences === null ) {

…point.

techanvil

Hi @ankitrox, thanks for making these changes.

There are just a couple of aspects that were missed from previous comments:

From #8606 (comment):

Therefore, please can you update this condition as follows, and please also add a bit of test coverage for this aspect

As per the quoted point, please can you add a bit of test coverage for the changes included via the getAvailableAudiences() resolver? I.e. that it should sync the audiences when the setting is null.

From #8606 (review):

Also, the QA Brief will need an update as it's currently focused on the change to createAudience() which will be removed. This should instead reflect the change to getAvailableAudiences() (i.e. the sync should be triggered when the value is initially null), and it's also worth including a QAB point to give the AudienceTiles component a quick check in Storybook to verify it's unchanged.

As per the above, please can you updated the QA Brief accordingly. Thanks!

ankitrox · 2024-04-30T07:55:48Z

Hi @ankitrox, thanks for making these changes.

There are just a couple of aspects that were missed from previous comments:

From #8606 (comment):

Therefore, please can you update this condition as follows, and please also add a bit of test coverage for this aspect

As per the quoted point, please can you add a bit of test coverage for the changes included via the getAvailableAudiences() resolver? I.e. that it should sync the audiences when the setting is null.

From #8606 (review):

Also, the QA Brief will need an update as it's currently focused on the change to createAudience() which will be removed. This should instead reflect the change to getAvailableAudiences() (i.e. the sync should be triggered when the value is initially null), and it's also worth including a QAB point to give the AudienceTiles component a quick check in Storybook to verify it's unchanged.

As per the above, please can you updated the QA Brief accordingly. Thanks!

Thank you @techanvil .

I have addressed following two points.

Added the test for getAvailableAudiences resolver that it should (and should not call) sync-audiences endpoint.
Added the QAB for getAvailableAudiences that it should call sync-audiences endpoint.
Added QAB for comparing AudienceTiles component in storybook.

techanvil

Thanks @ankitrox, this LGTM!

aaemnnosttv · 2024-05-03T16:36:36Z

assets/js/modules/analytics-4/datastore/audiences.js

-	 */
-	getAudiences( state ) {
-		return state.audiences;
+		return registry.select( MODULES_ANALYTICS_4 ).getAvailableAudiences();


@techanvil @ankitrox resolvers don't have a return value :) This line can simply be removed.

@aaemnnosttv thanks for spotting that, I totally missed it at review time. I'm including a fix in my forthcoming PR for #8131.

aaemnnosttv · 2024-05-03T16:38:40Z

assets/js/modules/analytics-4/datastore/audiences.test.js

+				fetchMock.postOnce( syncAvailableAudiencesEndpoint, {
+					body: availableAudiences,
+					status: 200,
+				} );


Using fetchMock here shouldn't be necessary since the expectation is for it not to be fetched. @ankitrox

Thanks here too, @aaemnnosttv, I am removing this call in my forthcoming PR too.

ankitrox added 4 commits April 23, 2024 20:03

Remove GET:audiences endpoint and getAudiences selector.

532d02f

Merge branch 'develop' into enhancement/8487-remove-get-audiences-end…

e5da1df

…point.

Add getAvailableAudiences base resolver.

8477992

Use setAvailableAudiences instead of receiveGetAudiences.

b30ea7f

ankitrox changed the title ~~Remove GET:audiences endpoint and getAudiences selector.~~ Remove GET:audiences endpoint and getAudiences selector Apr 23, 2024

Fix php and js tests.

4c284c3

ankitrox marked this pull request as ready for review April 24, 2024 06:03

Remove redundant code.

2d23e8e

ankitrox added 2 commits April 24, 2024 13:01

Fix: Failed storytbook test.

5df5a29

Return audiences from resolver.

4c19c61

techanvil reviewed Apr 24, 2024

View reviewed changes

ankitrox and others added 3 commits April 24, 2024 17:29

Update assets/js/modules/analytics-4/components/dashboard/AudienceSeg…

16cd135

…mentation/AudienceTiles.stories.js Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

Update assets/js/modules/analytics-4/components/dashboard/AudienceSeg…

f8c604e

…mentation/AudienceTiles.js Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

Update includes/Modules/Analytics_4.php

a62f099

Co-authored-by: Tom Rees-Herdman <tom.rees-herdman@10up.com>

techanvil reviewed Apr 24, 2024

View reviewed changes

ankitrox added 5 commits April 24, 2024 17:54

Update audienceFixture structure.

23dacc5

Fix: PHP tests.

18e41d4

Replace audienceName with audienceSlug.

f0d1e2d

Pass mapped audiences data to tests.

8329236

Fix: VRT failed test.

95b40cb

Add support of audienceSlug to tooltip component.

techanvil reviewed Apr 25, 2024

View reviewed changes

techanvil requested changes Apr 26, 2024

View reviewed changes

ankitrox added 3 commits April 29, 2024 14:39

Update slug for test users.

759429a

Fix: condition for audience check.

e8223d2

Update reference images for VRT.

ef280ad

ankitrox requested a review from techanvil April 29, 2024 09:11

ankitrox added 2 commits April 29, 2024 21:32

Merge branch 'develop' into enhancement/8487-remove-get-audiences-end…

c9c4fe1

…point.

Resolve conflicts for VRT reference files.

78701f5

techanvil requested changes Apr 29, 2024

View reviewed changes

Add tests for getAvailableAudiences resolver.

4e8f8fd

ankitrox requested a review from techanvil April 30, 2024 07:57

techanvil added 3 commits April 30, 2024 10:06

Fix "All visitors" display name.

665c671

Fix test description initial capitalisation.

273dae6

Improve test descriptions.

e68de59

techanvil merged commit 810122a into develop Apr 30, 2024
24 of 25 checks passed

techanvil deleted the enhancement/8487-remove-get-audiences-endpoint branch April 30, 2024 09:43

techanvil reviewed Apr 30, 2024

View reviewed changes

aaemnnosttv reviewed May 3, 2024

View reviewed changes

techanvil added a commit that referenced this pull request May 9, 2024

Address a couple of minor issues flagged on PR #8606 post-merge.

60e98ea

techanvil added a commit that referenced this pull request May 10, 2024

Address a couple of minor issues flagged on PR #8606 post-merge.

d8110e2

techanvil mentioned this pull request May 10, 2024

Implement the happy path for creating audiences and the custom dimension via the Setup CTA Banner. #8694

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove GET:audiences endpoint and getAudiences selector #8606

Remove GET:audiences endpoint and getAudiences selector #8606

ankitrox commented Apr 23, 2024 •

edited by techanvil

github-actions bot commented Apr 24, 2024 •

edited

github-actions bot commented Apr 24, 2024 •

edited

techanvil Apr 24, 2024 •

edited

techanvil Apr 24, 2024

techanvil Apr 24, 2024

techanvil Apr 24, 2024

ankitrox Apr 24, 2024

techanvil Apr 24, 2024

techanvil Apr 25, 2024

techanvil Apr 25, 2024

techanvil left a comment

techanvil Apr 26, 2024

techanvil Apr 26, 2024

techanvil left a comment

ankitrox commented Apr 30, 2024

techanvil left a comment

aaemnnosttv May 3, 2024

techanvil May 9, 2024 •

edited

aaemnnosttv May 3, 2024

techanvil May 9, 2024 •

edited

	.setAvailableAudiences( { audiences: audiencesFixture } );
	.setAvailableAudiences( mappedCachedAudiences );

	'displayName' => ( 'All Users' === $display_name ) ? __( 'All Visitors', 'google-site-kit' ) : $display_name,
	'displayName' => ( 'All Users' === $display_name ) ? 'All visitors' : $display_name,

	// Return an empty string for user defined audiences.
	return '';


		// If available audiences not present, sync the audience in state.
		if ( audiences === undefined ) {

Remove GET:audiences endpoint and getAudiences selector #8606

Remove GET:audiences endpoint and getAudiences selector #8606

Conversation

ankitrox commented Apr 23, 2024 • edited by techanvil

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Apr 24, 2024 • edited

github-actions bot commented Apr 24, 2024 • edited

techanvil Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

ankitrox commented Apr 30, 2024

techanvil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil May 9, 2024 • edited

Choose a reason for hiding this comment

ankitrox commented Apr 23, 2024 •

edited by techanvil

github-actions bot commented Apr 24, 2024 •

edited

github-actions bot commented Apr 24, 2024 •

edited

techanvil Apr 24, 2024 •

edited

techanvil May 9, 2024 •

edited

techanvil May 9, 2024 •

edited