Merge pull request #7444 from google/enhancement/#5118-correct-amp-mode

Detect AMP modes correctly
google · Aug 21, 2023 · 4ae5c57 · 4ae5c57
2 parents be0d6a7 + 46ef775
commit 4ae5c57
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 135 deletions.
diff --git a/assets/js/modules/tagmanager/components/common/AMPContainerSelect.js b/assets/js/modules/tagmanager/components/common/AMPContainerSelect.js
@@ -59,9 +59,6 @@ export default function AMPContainerSelect( { hasModuleAccess } ) {
 		return select( MODULES_TAGMANAGER ).getAMPContainers( accountID );
 	} );
 	const isAMP = useSelect( ( select ) => select( CORE_SITE ).isAMP() );
-	const isSecondaryAMP = useSelect( ( select ) =>
-		select( CORE_SITE ).isSecondaryAMP()
-	);
 
 	const { setAMPContainerID, setInternalAMPContainerID } =
 		useDispatch( MODULES_TAGMANAGER );
@@ -95,9 +92,7 @@ export default function AMPContainerSelect( { hasModuleAccess } ) {
 		return null;
 	}
 
-	const label = isSecondaryAMP
-		? __( 'AMP Container', 'google-site-kit' )
-		: __( 'Container', 'google-site-kit' );
+	const label = __( 'AMP Container', 'google-site-kit' );
 
 	if ( hasModuleAccess === false ) {
 		return (

diff --git a/assets/js/modules/tagmanager/components/common/AMPContainerSelect.test.js b/assets/js/modules/tagmanager/components/common/AMPContainerSelect.test.js
@@ -27,10 +27,7 @@ import {
 	CONTEXT_AMP,
 	CONTAINER_CREATE,
 } from '../../datastore/constants';
-import {
-	AMP_MODE_PRIMARY,
-	AMP_MODE_SECONDARY,
-} from '../../../../googlesitekit/datastore/site/constants';
+import { AMP_MODE_PRIMARY } from '../../../../googlesitekit/datastore/site/constants';
 import {
 	createTestRegistry,
 	freezeFetch,
@@ -250,55 +247,6 @@ describe( 'AMPContainerSelect', () => {
 		expect( queryByRole( 'progressbar' ) ).toBeInTheDocument();
 	} );
 
-	it( 'should be labeled as "Container" in a primary AMP context', () => {
-		const { account, containers } = factories.buildAccountWithContainers();
-		const accountID = account.accountId; // eslint-disable-line sitekit/acronym-case
-		registry.dispatch( MODULES_TAGMANAGER ).setAccountID( accountID );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetAccounts( [ account ] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getAccounts', [] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetContainers( containers, { accountID } );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getContainers', [ accountID ] );
-
-		const { container } = render( <AMPContainerSelect />, { registry } );
-
-		expect(
-			container.querySelector( '.mdc-floating-label' )
-		).toHaveTextContent( /Container/i );
-	} );
-
-	it( 'should be labeled as "AMP Container" in a secondary AMP context', () => {
-		const { account, containers } = factories.buildAccountWithContainers();
-		const accountID = account.accountId; // eslint-disable-line sitekit/acronym-case
-		registry.dispatch( MODULES_TAGMANAGER ).setAccountID( accountID );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetAccounts( [ account ] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getAccounts', [] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetContainers( containers, { accountID } );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getContainers', [ accountID ] );
-		provideSiteInfo( registry, { ampMode: AMP_MODE_SECONDARY } );
-
-		const { container } = render( <AMPContainerSelect />, { registry } );
-
-		expect(
-			container.querySelector( '.mdc-floating-label' )
-		).toHaveTextContent( /AMP Container/i );
-	} );
-
 	it( 'should render nothing in a non-AMP context', () => {
 		const account = factories.accountBuilder();
 		registry

diff --git a/assets/js/modules/tagmanager/components/common/WebContainerNameTextField.js b/assets/js/modules/tagmanager/components/common/WebContainerNameTextField.js
@@ -48,9 +48,7 @@ export default function WebContainerNameTextField() {
 	const siteName = useSelect( ( select ) =>
 		select( CORE_SITE ).getSiteName()
 	);
-	const isSecondaryAMP = useSelect( ( select ) =>
-		select( CORE_SITE ).isSecondaryAMP()
-	);
+	const isAMP = useSelect( ( select ) => select( CORE_SITE ).isAMP() );
 	const referenceSiteURL = useSelect( ( select ) =>
 		select( CORE_SITE ).getReferenceSiteURL()
 	);
@@ -76,7 +74,7 @@ export default function WebContainerNameTextField() {
 		return null;
 	}
 
-	const label = isSecondaryAMP
+	const label = isAMP
 		? __( 'Web Container Name', 'google-site-kit' )
 		: __( 'Container Name', 'google-site-kit' );
 

diff --git a/assets/js/modules/tagmanager/components/common/WebContainerSelect.js b/assets/js/modules/tagmanager/components/common/WebContainerSelect.js
@@ -58,12 +58,7 @@ export default function WebContainerSelect( { hasModuleAccess } ) {
 
 		return select( MODULES_TAGMANAGER ).getWebContainers( accountID );
 	} );
-	const isPrimaryAMP = useSelect( ( select ) =>
-		select( CORE_SITE ).isPrimaryAMP()
-	);
-	const isSecondaryAMP = useSelect( ( select ) =>
-		select( CORE_SITE ).isSecondaryAMP()
-	);
+	const isAMP = useSelect( ( select ) => select( CORE_SITE ).isAMP() );
 
 	const { setContainerID, setInternalContainerID } =
 		useDispatch( MODULES_TAGMANAGER );
@@ -88,11 +83,7 @@ export default function WebContainerSelect( { hasModuleAccess } ) {
 		[ containerID, setContainerID, setInternalContainerID, viewContext ]
 	);
 
-	if ( isPrimaryAMP ) {
-		return null;
-	}
-
-	const label = isSecondaryAMP
+	const label = isAMP
 		? __( 'Web Container', 'google-site-kit' )
 		: __( 'Container', 'google-site-kit' );
 

diff --git a/assets/js/modules/tagmanager/components/common/WebContainerSelect.test.js b/assets/js/modules/tagmanager/components/common/WebContainerSelect.test.js
@@ -28,8 +28,8 @@ import {
 	CONTAINER_CREATE,
 } from '../../datastore/constants';
 import {
-	AMP_MODE_SECONDARY,
 	AMP_MODE_PRIMARY,
+	AMP_MODE_SECONDARY,
 } from '../../../../googlesitekit/datastore/site/constants';
 import {
 	createTestRegistry,
@@ -284,48 +284,36 @@ describe( 'WebContainerSelect', () => {
 		).toHaveTextContent( /Container/i );
 	} );
 
-	it( 'should be labeled as "Web Container" in a secondary AMP context', () => {
-		const { account, containers } = factories.buildAccountWithContainers();
-		const accountID = account.accountId; // eslint-disable-line sitekit/acronym-case
-		registry.dispatch( MODULES_TAGMANAGER ).setAccountID( accountID );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetAccounts( [ account ] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getAccounts', [] );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetContainers( containers, { accountID } );
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.finishResolution( 'getContainers', [ accountID ] );
-		provideSiteInfo( registry, { ampMode: AMP_MODE_SECONDARY } );
-
-		const { container } = render( <WebContainerSelect />, { registry } );
-
-		expect(
-			container.querySelector( '.mdc-floating-label' )
-		).toHaveTextContent( /Web Container/i );
-	} );
-
-	it( 'should render nothing in a primary AMP context', () => {
-		const account = factories.accountBuilder();
-		registry
-			.dispatch( MODULES_TAGMANAGER )
-			.receiveGetAccounts( [ account ] );
-		provideSiteInfo( registry, { ampMode: AMP_MODE_PRIMARY } );
-
-		const { queryByRole, container } = render( <WebContainerSelect />, {
-			registry,
-		} );
+	it.each( [ AMP_MODE_PRIMARY, AMP_MODE_SECONDARY ] )(
+		'should be labeled as "Web Container" in a %s AMP context',
+		( ampMode ) => {
+			const { account, containers } =
+				factories.buildAccountWithContainers();
+			const accountID = account.accountId; // eslint-disable-line sitekit/acronym-case
+			registry.dispatch( MODULES_TAGMANAGER ).setAccountID( accountID );
+			registry
+				.dispatch( MODULES_TAGMANAGER )
+				.receiveGetAccounts( [ account ] );
+			registry
+				.dispatch( MODULES_TAGMANAGER )
+				.finishResolution( 'getAccounts', [] );
+			registry
+				.dispatch( MODULES_TAGMANAGER )
+				.receiveGetContainers( containers, { accountID } );
+			registry
+				.dispatch( MODULES_TAGMANAGER )
+				.finishResolution( 'getContainers', [ accountID ] );
+			provideSiteInfo( registry, { ampMode } );
+
+			const { container } = render( <WebContainerSelect />, {
+				registry,
+			} );
 
-		expect( queryByRole( 'progressbar' ) ).not.toBeInTheDocument();
-		expect(
-			queryByRole( 'menu', { hidden: true } )
-		).not.toBeInTheDocument();
-		expect( container ).toBeEmptyDOMElement();
-	} );
+			expect(
+				container.querySelector( '.mdc-floating-label' )
+			).toHaveTextContent( /Web Container/i );
+		}
+	);
 
 	it( 'should disable the web container select if the user does not have module access', () => {
 		const { account } = factories.buildAccountWithContainers();

diff --git a/includes/Context.php b/includes/Context.php
@@ -28,11 +28,9 @@ class Context {
 	/**
 	 * Primary "standard" AMP website mode.
 	 *
-	 * This mode is currently unused due to Tag Manager setup not showing the Web Container dropdown
-	 * when AMP is in standard mode and some urls have AMP disabled.
-	 *
 	 * @since 1.0.0 Originally introduced.
 	 * @since 1.36.0 Marked as unused, see description.
+	 * @since n.e.x.t Removed the description and reinstated.
 	 * @var string
 	 */
 	const AMP_MODE_PRIMARY = 'primary';
@@ -317,18 +315,37 @@ public function is_amp() {
 	 * Gets the current AMP mode.
 	 *
 	 * @since 1.0.0
+	 * @since n.e.x.t Extracted AMP plugin related logic to `get_amp_mode_from_amp_plugin` function.
 	 *
 	 * @return bool|string 'primary' if in standard mode,
-	 *                     'secondary' if in transitional or reader modes
+	 *                     'secondary' if in transitional or reader modes, or the Web Stories plugin is active
 	 *                     false if AMP not active, or unknown mode
 	 */
 	public function get_amp_mode() {
-		// If the Web Stories plugin is enabled, consider the site to be running
-		// in Secondary AMP mode.
-		if ( defined( 'WEBSTORIES_VERSION' ) ) {
-			return self::AMP_MODE_SECONDARY;
+		$amp_mode = $this->get_amp_mode_from_amp_plugin();
+
+		if ( false === $amp_mode ) {
+			// If the Web Stories plugin is enabled, consider the site to be running
+			// in Secondary AMP mode.
+			if ( defined( 'WEBSTORIES_VERSION' ) ) {
+				return self::AMP_MODE_SECONDARY;
+			}
 		}
 
+		return $amp_mode;
+	}
+
+	/**
+	 * Gets the current AMP mode from the AMP plugin.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @return bool|string 'primary' if in standard mode,
+	 *                     'secondary' if in transitional or reader modes
+	 *                     false if AMP not active, or unknown mode
+	 */
+	private function get_amp_mode_from_amp_plugin() {
+
 		if ( ! class_exists( 'AMP_Theme_Support' ) ) {
 			return false;
 		}
@@ -366,20 +383,19 @@ public function get_amp_mode() {
 				$mode = AMP_Theme_Support::get_support_mode();
 			}
 
-			if (
-				in_array(
-					$mode,
-					array(
-						AMP_Theme_Support::STANDARD_MODE_SLUG,
-						AMP_Theme_Support::TRANSITIONAL_MODE_SLUG,
-						AMP_Theme_Support::READER_MODE_SLUG,
-					),
-					true
-				)
-			) {
+			if ( AMP_Theme_Support::STANDARD_MODE_SLUG === $mode ) {
+				return self::AMP_MODE_PRIMARY;
+			}
+
+			if ( in_array( $mode, array( AMP_Theme_Support::TRANSITIONAL_MODE_SLUG, AMP_Theme_Support::READER_MODE_SLUG ), true ) ) {
 				return self::AMP_MODE_SECONDARY;
 			}
 		} elseif ( function_exists( 'amp_is_canonical' ) ) {
+			// On older versions, if it is not primary AMP, it is definitely secondary AMP (transitional or reader mode).
+			if ( amp_is_canonical() ) {
+				return self::AMP_MODE_PRIMARY;
+			}
+
 			return self::AMP_MODE_SECONDARY;
 		}