Merge branch 'develop' into fix/1140-duplicate-dashboard-html

Author: felixarntz

assets/js/modules/tagmanager/setup.js

@@ -0,0 +1,16 @@
+/**
+ * Gets an object of utility functions for filtering the given list of containers.
+ *
+ * @param {Array} containers Containers to filter.
+ * @return {Object} Object with keys mapping to utility functions to operate on given containers.
+ */
+export default function getContainers( containers ) {
+	return {
+		/**
+		 * Gets containers that include the given usage context.
+		 * @param {string} context The context to filter by.
+		 * @return {Array} Containers with the given usage context.
+		 */
+		byContext: ( context ) => containers.filter( ( c ) => c.usageContext.includes( context ) ),
+	};
+}

assets/js/modules/tagmanager/util/get-containers.js

@@ -1,3 +1,4 @@
+export { default as getContainers } from './get-containers';
 export { default as isValidAccountID } from './is-valid-account-id';
 export { default as isValidContainerID } from './is-valid-container-id';
 export { default as tagMatchers } from './tag-matchers';

assets/js/modules/tagmanager/util/index.js

@@ -264,17 +264,15 @@ private function setup_client() {
 
 		// This is called when the client refreshes the access token on-the-fly.
 		$client->setTokenCallback(
-			function( $cache_key, $access_token ) {
+			function( $cache_key, $access_token ) use ( $client ) {
 				$expires_in = HOUR_IN_SECONDS; // Sane default, Google OAuth tokens are typically valid for an hour.
 				$created    = 0; // This will be replaced with the current timestamp when saving.
 
 				// Try looking up the real values if possible.
-				if ( isset( $client ) ) {
-					$token = $client->getAccessToken();
-					if ( isset( $token['access_token'], $token['expires_in'], $token['created'] ) && $access_token === $token['access_token'] ) {
-						$expires_in = $token['expires_in'];
-						$created    = $token['created'];
-					}
+				$token = $client->getAccessToken();
+				if ( isset( $token['access_token'], $token['expires_in'], $token['created'] ) && $access_token === $token['access_token'] ) {
+					$expires_in = $token['expires_in'];
+					$created    = $token['created'];
 				}
 
 				$this->set_access_token( $access_token, $expires_in, $created );

includes/Core/Authentication/Clients/OAuth_Client.php

@@ -706,27 +706,12 @@ private function parse_info( array $info ) {
 	 * @param string    $datapoint Datapoint originally requested.
 	 * @return WP_Error WordPress error object.
 	 */
-	private function exception_to_error( Exception $e, $datapoint ) {
+	protected function exception_to_error( Exception $e, $datapoint ) {
 		$code = $e->getCode();
 		if ( empty( $code ) ) {
 			$code = 'unknown';
 		}
 
-		// This is not great to have here, but is completely internal so it can be improved/removed at any time.
-		if ( $this instanceof \Google\Site_Kit\Modules\AdSense ) {
-			switch ( $datapoint ) {
-				case 'accounts':
-				case 'alerts':
-				case 'clients':
-				case 'urlchannels':
-					$errors = json_decode( $e->getMessage() );
-					if ( $errors ) {
-						return new \WP_Error( $e->getCode(), $errors, array( 'status' => 500 ) );
-					}
-					break;
-			}
-		}
-
 		$reason = '';
 
 		if ( $e instanceof Google_Service_Exception ) {

includes/Core/Modules/Module.php

@@ -10,6 +10,7 @@
 
 namespace Google\Site_Kit\Modules;
 
+use Exception;
 use Google\Site_Kit\Core\Modules\Module;
 use Google\Site_Kit\Core\Modules\Module_Settings;
 use Google\Site_Kit\Core\Modules\Module_With_Screen;
@@ -747,4 +748,24 @@ protected function setup_services( Google_Site_Kit_Client $client ) {
 	protected function setup_settings() {
 		return new Settings( $this->options );
 	}
+
+	/**
+	 * Transforms an exception into a WP_Error object.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param Exception $e         Exception object.
+	 * @param string    $datapoint Datapoint originally requested.
+	 * @return WP_Error WordPress error object.
+	 */
+	protected function exception_to_error( Exception $e, $datapoint ) {
+		if ( in_array( $datapoint, array( 'accounts', 'alerts', 'clients', 'urlchannels' ), true ) ) {
+			$errors = json_decode( $e->getMessage() );
+			if ( $errors ) {
+				return new WP_Error( $e->getCode(), $errors, array( 'status' => 500 ) );
+			}
+		}
+
+		return parent::exception_to_error( $e, $datapoint );
+	}
 }

includes/Modules/AdSense.php

@@ -493,40 +493,40 @@ protected function create_data_request( Data_Request $data ) {
 				}
 				return $this->get_tagmanager_service()->accounts_containers->listAccountsContainers( "accounts/{$data['accountID']}" );
 			case 'POST:settings':
-				if ( ! isset( $data['accountID'] ) ) {
-					/* translators: %s: Missing parameter name */
-					return new WP_Error( 'missing_required_param', sprintf( __( 'Request parameter is empty: %s.', 'google-site-kit' ), 'accountID' ), array( 'status' => 400 ) );
+				$required_params = array( 'accountID', 'usageContext' );
+
+				if ( self::USAGE_CONTEXT_WEB === $data['usageContext'] ) { // No AMP.
+					$required_params[] = $this->context_map[ self::USAGE_CONTEXT_WEB ];
+				} elseif ( self::USAGE_CONTEXT_AMP === $data['usageContext'] ) { // Primary AMP.
+					$required_params[] = $this->context_map[ self::USAGE_CONTEXT_AMP ];
+				} else { // Secondary AMP.
+					array_push( $required_params, ...array_values( $this->context_map ) );
 				}
 
-				$usage_context = $data['usageContext'] ?: self::USAGE_CONTEXT_WEB;
-
-				if ( self::USAGE_CONTEXT_WEB === $usage_context && ! isset( $data['containerID'] ) ) {
-					/* translators: %s: Missing parameter name */
-					return new WP_Error( 'missing_required_param', sprintf( __( 'Request parameter is empty: %s.', 'google-site-kit' ), 'containerID' ), array( 'status' => 400 ) );
-				}
-				if ( self::USAGE_CONTEXT_AMP === $usage_context && ! isset( $data['ampContainerID'] ) ) {
-					/* translators: %s: Missing parameter name */
-					return new WP_Error( 'missing_required_param', sprintf( __( 'Request parameter is empty: %s.', 'google-site-kit' ), 'ampContainerID' ), array( 'status' => 400 ) );
+				foreach ( $required_params as $required_param ) {
+					if ( ! isset( $data[ $required_param ] ) ) {
+						/* translators: %s: Missing parameter name */
+						return new WP_Error( 'missing_required_param', sprintf( __( 'Request parameter is empty: %s.', 'google-site-kit' ), $required_param ), array( 'status' => 400 ) );
+					}
 				}
 
-				return function() use ( $data, $usage_context ) {
-					$option        = $data->data;
-					$container_key = $this->context_map[ $usage_context ];
-					$container_id  = $data[ $container_key ];
-
-					if ( 'container_create' === $container_id ) {
-						$create_container_response = $this->create_container( $data['accountID'], $usage_context );
+				return function() use ( $data ) {
+					$option = $data->data;
 
-						if ( is_wp_error( $create_container_response ) ) {
-							return $create_container_response;
+					try {
+						if ( 'container_create' === $data['containerID'] ) {
+							$option['containerID'] = $this->create_container( $data['accountID'], self::USAGE_CONTEXT_WEB );
 						}
-
-						$option[ $container_key ] = $create_container_response;
+						if ( 'container_create' === $data['ampContainerID'] ) {
+							$option['ampContainerID'] = $this->create_container( $data['accountID'], self::USAGE_CONTEXT_AMP );
+						}
+					} catch ( Exception $e ) {
+						return $this->exception_to_error( $e, $data->datapoint );
 					}
 
 					$this->get_settings()->merge( $option );
 
-					return $option;
+					return $this->get_settings()->get();
 				};
 			case 'GET:tag-permission':
 				return function () use ( $data ) {
@@ -566,38 +566,37 @@ protected function create_data_request( Data_Request $data ) {
 	 * Creates GTM Container.
 	 *
 	 * @since 1.0.0
-	 * @param string       $account_id The account ID.
+	 * @param string       $account_id    The account ID.
 	 * @param string|array $usage_context The container usage context(s).
 	 *
-	 * @return mixed Container ID on success, or WP_Error on failure.
+	 * @return string Container public ID.
+	 * @throws Exception Throws an exception if raised during container creation.
 	 */
 	protected function create_container( $account_id, $usage_context = self::USAGE_CONTEXT_WEB ) {
 		$restore_defer = $this->with_client_defer( false );
 
 		// Use site name for container, fallback to domain of reference URL.
 		$container_name = get_bloginfo( 'name' ) ?: wp_parse_url( $this->context->get_reference_site_url(), PHP_URL_HOST );
+		// Prevent naming conflict (Tag Manager does not allow more than one with same name).
+		if ( self::USAGE_CONTEXT_AMP === $usage_context ) {
+			$container_name .= ' AMP';
+		}
 		$container_name = self::sanitize_container_name( $container_name );
 
 		$container = new Google_Service_TagManager_Container();
 		$container->setName( $container_name );
 		$container->setUsageContext( (array) $usage_context );
 
 		try {
-			$container = $this->get_tagmanager_service()->accounts_containers->create( "accounts/{$account_id}", $container );
-		} catch ( Google_Service_Exception $e ) {
-			$restore_defer();
-			$message = $e->getErrors();
-			if ( isset( $message[0]['message'] ) ) {
-				$message = $message[0]['message'];
-			}
-			return new WP_Error( $e->getCode(), $message );
-		} catch ( Exception $e ) {
+			$new_container = $this->get_tagmanager_service()->accounts_containers->create( "accounts/{$account_id}", $container );
+		} catch ( Exception $exception ) {
 			$restore_defer();
-			return new WP_Error( $e->getCode(), $e->getMessage() );
+			throw $exception;
 		}
 
 		$restore_defer();
-		return $container->getPublicId();
+
+		return $new_container->getPublicId();
 	}
 
 	/**
@@ -646,27 +645,15 @@ protected function parse_data_response( Data_Request $data, $response ) {
 				return array_merge( $response, compact( 'containers' ) );
 			case 'GET:containers':
 				/* @var Google_Service_TagManager_ListContainersResponse $response Response object. */
-				$account_id    = $data['accountID'];
 				$usage_context = $data['usageContext'] ?: self::USAGE_CONTEXT_WEB;
 				/* @var Google_Service_TagManager_Container[] $containers Filtered containers. */
 				$containers = array_filter(
 					(array) $response->getContainer(),
 					function ( Google_Service_TagManager_Container $container ) use ( $usage_context ) {
-						return in_array( $usage_context, $container->getUsageContext(), true );
+						return array_intersect( (array) $usage_context, $container->getUsageContext() );
 					}
 				);
 
-				if ( ! $containers && $account_id ) {
-					// If no containers, attempt to create a new container.
-					$new_container = $this->create_container( $account_id, $usage_context );
-
-					if ( is_wp_error( $new_container ) ) {
-						return $new_container;
-					}
-
-					return $this->get_data( 'containers', array( 'accountID' => $account_id ) );
-				}
-
 				return array_values( $containers );
 		}
 
@@ -693,7 +680,13 @@ function ( Google_Service_TagManager_Container $container ) use ( $usage_context
 	private function get_account_for_container( $container_id, $accounts ) {
 		foreach ( (array) $accounts as $account ) {
 			/* @var Google_Service_TagManager_Account $account Tag manager account */
-			$containers = $this->get_data( 'containers', array( 'accountID' => $account->getAccountId() ) );
+			$containers = $this->get_data(
+				'containers',
+				array(
+					'accountID'    => $account->getAccountId(),
+					'usageContext' => array_keys( $this->context_map ),
+				)
+			);
 
 			if ( is_wp_error( $containers ) ) {
 				break;

includes/Modules/Tag_Manager.php

@@ -19,11 +19,15 @@
 const ACCOUNT_ID_A = '100';
 const ACCOUNT_ID_B = '101';
 
-const PUBLIC_ID_X = 'GTM-ABCXYZ';
-const PUBLIC_ID_Y = 'GTM-BCDWXY';
+const PUBLIC_ID_X  = 'GTM-ABCXYZ';
+const PUBLIC_ID_Y  = 'GTM-BCDWXY';
+const PUBLIC_ID_AX = 'GTM-AMPXYZ';
+const PUBLIC_ID_AY = 'GTM-AMPWXY';
 
-const CONTAINER_ID_X = '200';
-const CONTAINER_ID_Y = '201';
+const CONTAINER_ID_X  = '200';
+const CONTAINER_ID_Y  = '201';
+const CONTAINER_ID_AX = '210';
+const CONTAINER_ID_AY = '211';
 
 function filter_by_account_id( $items, $account_id ) {
 	return array_values(
@@ -36,6 +40,17 @@ function ( $item ) use ( $account_id ) {
 	);
 }
 
+function filter_by_context( $items, $context ) {
+	return array_values(
+		array_filter(
+			$items,
+			function ( $item ) use ( $context ) {
+				return (bool) array_intersect( (array) $context, $item['usageContext'] );
+			}
+		)
+	);
+}
+
 add_action(
 	'rest_api_init',
 	function () {
@@ -52,16 +67,32 @@ function () {
 		);
 		$containers = array(
 			array(
-				'accountId'   => ACCOUNT_ID_A,
-				'publicId'    => PUBLIC_ID_X,
-				'containerId' => CONTAINER_ID_X,
-				'name'        => 'Test Container X',
+				'accountId'    => ACCOUNT_ID_A,
+				'publicId'     => PUBLIC_ID_X,
+				'containerId'  => CONTAINER_ID_X,
+				'name'         => 'Test Container X',
+				'usageContext' => array( 'web' ),
+			),
+			array(
+				'accountId'    => ACCOUNT_ID_B,
+				'publicId'     => PUBLIC_ID_Y,
+				'containerId'  => CONTAINER_ID_Y,
+				'name'         => 'Test Container Y',
+				'usageContext' => array( 'web' ),
 			),
 			array(
-				'accountId'   => ACCOUNT_ID_B,
-				'publicId'    => PUBLIC_ID_Y,
-				'containerId' => CONTAINER_ID_Y,
-				'name'        => 'Test Container Y',
+				'accountId'    => ACCOUNT_ID_A,
+				'publicId'     => PUBLIC_ID_AX,
+				'containerId'  => CONTAINER_ID_AX,
+				'name'         => 'Test AMP Container AX',
+				'usageContext' => array( 'amp' ),
+			),
+			array(
+				'accountId'    => ACCOUNT_ID_B,
+				'publicId'     => PUBLIC_ID_AY,
+				'containerId'  => CONTAINER_ID_AY,
+				'name'         => 'Test AMP Container AY',
+				'usageContext' => array( 'amp' ),
 			),
 		);
 
@@ -84,6 +115,7 @@ function () {
 				'methods'  => 'GET',
 				'callback' => function ( $request ) use ( $accounts, $containers ) {
 					$account_id = $request['accountID'] ?: $accounts[0]['accountId'];
+					$containers = filter_by_context( $containers, $request['usageContext'] );
 
 					return array(
 						'accounts'   => $accounts,
@@ -100,6 +132,8 @@ function () {
 			array(
 				'methods'  => 'GET',
 				'callback' => function ( $request ) use ( $containers ) {
+					$containers = filter_by_context( $containers, $request['usageContext'] );
+
 					return filter_by_account_id( $containers, $request['accountID'] );
 				},
 			),

tests/e2e/plugins/module-setup-tagmanager.php

@@ -110,6 +110,10 @@ describe( 'Tag Manager module setup', () => {
 		await activatePlugin( 'e2e-tests-module-setup-tagmanager-api-mock' );
 		await proceedToTagManagerSetup();
 
+		// Ensure only web container select is shown.
+		await expect( page ).toMatchElement( '.googlesitekit-tagmanager__select-container--web' );
+		await expect( page ).not.toMatchElement( '.googlesitekit-tagmanager__select-container--amp' );
+
 		// Ensure account and container are selected by default.
 		await expect( page ).toMatchElement( '.googlesitekit-tagmanager__select-account .mdc-select__selected-text', { text: /test account a/i } );
 		await expect( page ).toMatchElement( '.googlesitekit-tagmanager__select-container .mdc-select__selected-text', { text: /test container x/i } );
@@ -121,9 +125,17 @@ describe( 'Tag Manager module setup', () => {
 			expect( page ).toClick( '.mdc-menu-surface--open .mdc-list-item', { text: /test account b/i } ),
 		] );
 
-		// Ensure proper account and container are now selected.
+		// Ensure account is selected.
 		await expect( page ).toMatchElement( '.googlesitekit-tagmanager__select-account .mdc-select__selected-text', { text: /test account b/i } );
-		await expect( page ).toMatchElement( '.googlesitekit-tagmanager__select-container .mdc-select__selected-text', { text: /test container y/i } );
+
+		// Select a container.
+		await expect( page ).toClick( '.googlesitekit-tagmanager__select-container' );
+		// Ensure no AMP containers are shown as options.
+		// expect(...).not.toMatchElement with textContent matching does not work as expected.
+		await expect(
+			await page.$$eval( '.mdc-menu-surface--open .mdc-list-item', ( nodes ) => !! nodes.find( ( e ) => e.textContent.match( /test amp container/i ) ) )
+		).toStrictEqual( false );
+		await expect( page ).toClick( '.mdc-menu-surface--open .mdc-list-item', { text: /test container y/i } );
 
 		await page.waitFor( 1000 );
 		await expect( page ).toClick( 'button', { text: /confirm \& continue/i } );