Merge pull request #10194 from google/fix/10187-main-foreach-error

Fix `forEach` error
google · Feb 10, 2025 · 370ca1c · 370ca1c
2 parents a54b75d + 93b4390
commit 370ca1c
Show file tree

Hide file tree

Showing 6 changed files with 227 additions and 155 deletions.
diff --git a/assets/js/modules/analytics-4/datastore/accounts.js b/assets/js/modules/analytics-4/datastore/accounts.js
@@ -45,10 +45,7 @@ import { actions as errorStoreActions } from '../../../googlesitekit/data/create
 import { createValidatedAction } from '../../../googlesitekit/data/utils';
 import { isValidAccountSelection } from '../utils/validation';
 import { caseInsensitiveListSort } from '../../../util/case-insensitive-sort';
-import {
-	populateAccountID,
-	populatePropertyAndAccountIds,
-} from '../utils/account';
+import { populateAccountSummaries } from '../utils/account';
 
 const { receiveError, clearError, clearErrors } = errorStoreActions;
 
@@ -73,7 +70,7 @@ const fetchGetAccountSummariesStore = createFetchStore( {
 			...state,
 			accountSummaries: [
 				...( state.accountSummaries || [] ),
-				...( response.accountSummaries || [] ),
+				...populateAccountSummaries( response.accountSummaries || [] ),
 			],
 		};
 	},
@@ -109,8 +106,7 @@ const START_SELECTING_ACCOUNT = 'START_SELECTING_ACCOUNT';
 const FINISH_SELECTING_ACCOUNT = 'FINISH_SELECTING_ACCOUNT';
 const RESET_ACCOUNT_SUMMARIES = 'RESET_ACCOUNT_SUMMARIES';
 const RESET_ACCOUNT_SETTINGS = 'RESET_ACCOUNT_SETTINGS';
-const TRANSFORM_AND_SORT_ACCOUNT_SUMMARIES =
-	'TRANSFORM_AND_SORT_ACCOUNT_SUMMARIES';
+const SORT_ACCOUNT_SUMMARIES = 'SORT_ACCOUNT_SUMMARIES';
 
 const baseInitialState = {
 	accountSummaries: undefined,
@@ -268,19 +264,15 @@ const baseActions = {
 	},
 
 	/**
-	 * Creates an action to transform and sort account summaries.
-	 *
-	 * This action is typically dispatched when account summaries need to be
-	 * transformed (e.g., extracting and populating relevant account and property
-	 * IDs) and then sorted in a case-insensitive manner by display name.
+	 * Sorts account summaries.
 	 *
-	 * @since 1.138.0
+	 * @since n.e.x.t
 	 *
-	 * @return {Object} The action object with the type `TRANSFORM_AND_SORT_ACCOUNT_SUMMARIES`.
+	 * @return {Object} Redux-style action.
 	 */
-	transformAndSortAccountSummaries() {
+	sortAccountSummaries() {
 		return {
-			type: TRANSFORM_AND_SORT_ACCOUNT_SUMMARIES,
+			type: SORT_ACCOUNT_SUMMARIES,
 		};
 	},
 };
@@ -311,30 +303,15 @@ const baseReducer = createReducer( ( state, { type } ) => {
 			state.settings.webDataStreamID = undefined;
 			break;
 
-		case TRANSFORM_AND_SORT_ACCOUNT_SUMMARIES:
+		case SORT_ACCOUNT_SUMMARIES:
 			if ( ! state.accountSummaries?.length ) {
-				return state;
+				return;
 			}
 
-			state.accountSummaries = state.accountSummaries.map(
-				( account ) => {
-					const accountObj = populateAccountID( account );
-					accountObj.propertySummaries = (
-						accountObj.propertySummaries || []
-					).map( ( property ) =>
-						populatePropertyAndAccountIds( property )
-					);
-
-					return accountObj;
-				}
-			);
-
 			state.accountSummaries = caseInsensitiveListSort(
 				state.accountSummaries,
 				'displayName'
 			);
-
-			return state;
 	}
 } );
 
@@ -361,7 +338,7 @@ const baseResolvers = {
 			} while ( nextPageToken );
 		}
 
-		yield baseActions.transformAndSortAccountSummaries();
+		yield baseActions.sortAccountSummaries();
 	},
 };
 

diff --git a/assets/js/modules/analytics-4/datastore/accounts.test.js b/assets/js/modules/analytics-4/datastore/accounts.test.js
@@ -31,7 +31,6 @@ import {
 	untilResolved,
 } from '../../../../../tests/js/utils';
 import * as fixtures from './__fixtures__';
-import { caseInsensitiveListSort } from '../../../util/case-insensitive-sort';
 
 describe( 'modules/analytics-4 accounts', () => {
 	let registry;
@@ -259,24 +258,30 @@ describe( 'modules/analytics-4 accounts', () => {
 			} );
 		} );
 
-		describe( 'transformAndSortAccountSummaries', () => {
-			it( 'should create an action to transform and sort account summaries', async () => {
-				registry
-					.dispatch( MODULES_ANALYTICS_4 )
-					.receiveGetAccountSummaries( fixtures.accountSummaries );
+		describe( 'sortAccountSummaries', () => {
+			it( 'should sort account summaries in state by their display names', async () => {
+				const summary = {
+					account: 'accounts/123',
+					_id: '123',
+					propertySummaries: [],
+				};
+				const summaryA = { displayName: 'Account A', ...summary };
+				const summaryB = { displayName: 'Account B', ...summary };
+				const summaryC = { displayName: 'Account C', ...summary };
+				const { receiveGetAccountSummaries, sortAccountSummaries } =
+					registry.dispatch( MODULES_ANALYTICS_4 );
 
-				await registry
-					.dispatch( MODULES_ANALYTICS_4 )
-					.transformAndSortAccountSummaries();
+				receiveGetAccountSummaries( {
+					accountSummaries: [ summaryC, summaryA, summaryB ],
+				} );
 
-				expect(
-					registry.select( MODULES_ANALYTICS_4 ).getAccountSummaries()
-				).toEqual(
-					caseInsensitiveListSort(
-						fixtures.accountSummaries.accountSummaries,
-						'displayName'
-					)
-				);
+				await sortAccountSummaries();
+
+				expect( store.getState().accountSummaries ).toEqual( [
+					summaryA,
+					summaryB,
+					summaryC,
+				] );
 			} );
 		} );
 	} );

diff --git a/assets/js/modules/analytics-4/datastore/webdatastreams.js b/assets/js/modules/analytics-4/datastore/webdatastreams.js
@@ -462,15 +462,17 @@ const baseSelectors = {
 			const info = {};
 			const propertyIDs = [];
 
-			summaries.forEach( ( { _id: accountID, propertySummaries } ) => {
-				propertySummaries.forEach( ( { _id: propertyID } ) => {
-					propertyIDs.push( propertyID );
-					info[ propertyID ] = {
-						accountID,
-						propertyID,
-					};
-				} );
-			} );
+			summaries.forEach(
+				( { _id: accountID, propertySummaries = [] } ) => {
+					propertySummaries.forEach( ( { _id: propertyID } ) => {
+						propertyIDs.push( propertyID );
+						info[ propertyID ] = {
+							accountID,
+							propertyID,
+						};
+					} );
+				}
+			);
 
 			if ( propertyIDs.length === 0 ) {
 				return null;

diff --git a/assets/js/modules/analytics-4/datastore/webdatastreams.test.js b/assets/js/modules/analytics-4/datastore/webdatastreams.test.js
@@ -36,6 +36,7 @@ import {
 } from '../../../../../tests/js/utils';
 import { MODULES_ANALYTICS_4 } from './constants';
 import * as fixtures from './__fixtures__';
+import { populateAccountSummaries } from '../utils/account';
 
 describe( 'modules/analytics-4 webdatastreams', () => {
 	let registry;
@@ -714,33 +715,39 @@ describe( 'modules/analytics-4 webdatastreams', () => {
 			const accountSummaries = {
 				accountSummaries: [
 					{
-						_id: '123456',
 						propertySummaries: [
-							{ _id: '1122334455' },
-							{ _id: '1122334456' },
-							{ _id: '1122334457' },
+							{ property: 'properties/1122334455' },
+							{ property: 'properties/1122334456' },
+							{ property: 'properties/1122334457' },
 						],
 						account: 'accounts/123456',
 					},
 					{
-						_id: '123457',
 						propertySummaries: [
-							{ _id: '1122334465' },
-							{ _id: '1122334466' },
+							{ property: 'properties/1122334465' },
+							{ property: 'properties/1122334466' },
 						],
 						account: 'accounts/123457',
 					},
 					{
-						_id: '123458',
-						propertySummaries: [ { _id: '1122334475' } ],
+						propertySummaries: [
+							{ property: 'properties/1122334475' },
+						],
 						account: 'accounts/123458',
 					},
+					{
+						account: 'accounts/123459',
+						// If an account has no properties, propertySummaries will not be set.
+						// This is important to test to catch cases that assume it is present.
+					},
 				],
 				nextPageToken: null,
 			};
 
-			const propertyIDs = accountSummaries.accountSummaries
-				.map( ( { propertySummaries } ) =>
+			const propertyIDs = populateAccountSummaries(
+				accountSummaries.accountSummaries
+			)
+				.map( ( { propertySummaries = [] } ) =>
 					propertySummaries.map( ( { _id } ) => _id )
 				)
 				.reduce( ( acc, propIDs ) => [ ...acc, ...propIDs ], [] );
@@ -808,9 +815,6 @@ describe( 'modules/analytics-4 webdatastreams', () => {
 
 			beforeEach( () => {
 				provideSiteInfo( registry );
-				registry.dispatch( MODULES_ANALYTICS_4 ).receiveGetSettings( {
-					accountID: 'UA-abcd',
-				} );
 				registry
 					.dispatch( MODULES_ANALYTICS_4 )
 					.receiveGetSettings( {} );

diff --git a/assets/js/modules/analytics-4/utils/account.js b/assets/js/modules/analytics-4/utils/account.js
@@ -107,3 +107,26 @@ export const populatePropertyAndAccountIds = ( property ) => {
 		_accountID,
 	};
 };
+
+/**
+ * Populates a list of accountSummaries with IDs for accounts and properties.
+ *
+ * @since n.e.x.t
+ *
+ * @param {Array|unknown} accountSummaries Account summaries to populate.
+ * @return {Array|unknown} Populated account summaries or given value if not an array.
+ */
+export const populateAccountSummaries = ( accountSummaries ) => {
+	if ( ! Array.isArray( accountSummaries ) ) {
+		return accountSummaries;
+	}
+
+	return accountSummaries.map( ( account ) => {
+		return {
+			...populateAccountID( account ),
+			propertySummaries: ( account.propertySummaries || [] ).map(
+				( property ) => populatePropertyAndAccountIds( property )
+			),
+		};
+	} );
+};