Only programmmatically select legacy theme when current Reader theme …

…is unavailable
ampproject · Aug 7, 2020 · 6bb44d7 · 6bb44d7
1 parent 17261a3
commit 6bb44d7
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 14 deletions.
diff --git a/assets/src/components/reader-themes-context-provider/index.js b/assets/src/components/reader-themes-context-provider/index.js
@@ -9,7 +9,7 @@ import { __ } from '@wordpress/i18n';
  * External dependencies
  */
 import PropTypes from 'prop-types';
-import { LEGACY_THEME_SLUG } from 'amp-settings';
+import { READER_THEME_AVAILABLE, LEGACY_THEME_SLUG } from 'amp-settings';
 
 /**
  * Internal dependencies
@@ -66,14 +66,9 @@ export function ReaderThemesContextProvider( { wpAjaxUrl, children, currentTheme
 				};
 			}
 
-			const themeIndexes = {};
-			themes.forEach( ( { slug }, index ) => {
-				themeIndexes[ slug ] = index;
-			} );
-
 			return {
-				originalSelectedTheme: originalReaderTheme in themeIndexes ? themes[ themeIndexes[ originalReaderTheme ] ] : emptyTheme,
-				selectedTheme: readerTheme in themeIndexes ? themes[ themeIndexes[ readerTheme ] ] : themes[ themeIndexes[ LEGACY_THEME_SLUG ] ],
+				originalSelectedTheme: themes.find( ( { slug } ) => slug === originalReaderTheme ) || emptyTheme,
+				selectedTheme: themes.find( ( { slug } ) => slug === readerTheme ) || emptyTheme,
 			};
 		},
 		[ originalReaderTheme, readerTheme, themes ],
@@ -86,8 +81,8 @@ export function ReaderThemesContextProvider( { wpAjaxUrl, children, currentTheme
 	const [ downloadingThemeError, setDownloadingThemeError ] = useState( null );
 
 	/**
-	 * If the currently selected theme is unavailable and not installable, the current theme is the active theme, or a
-	 * theme selection was not originally made, set the Reader theme to AMP Legacy.
+	 * If the currently selected theme is not installable, is the active theme, or unavailable for selection, set the
+	 * Reader theme to AMP Legacy.
 	 */
 	useEffect( () => {
 		if ( themeWasOverridden ) { // Only do this once.
@@ -97,7 +92,7 @@ export function ReaderThemesContextProvider( { wpAjaxUrl, children, currentTheme
 		if (
 			selectedTheme.availability === 'non-installable' ||
 			originalSelectedTheme.availability === 'active' ||
-			( ! originalSelectedTheme.slug && LEGACY_THEME_SLUG === selectedTheme.slug )
+			! READER_THEME_AVAILABLE
 		) {
 			updateOptions( { reader_theme: LEGACY_THEME_SLUG } );
 			setThemeWasOverridden( true );

diff --git a/src/Admin/OnboardingWizardSubmenuPage.php b/src/Admin/OnboardingWizardSubmenuPage.php
@@ -14,6 +14,7 @@
 use AmpProject\AmpWP\Infrastructure\Delayed;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
+use AmpProject\AmpWP\Option;
 use AmpProject\AmpWP\QueryVar;
 use AmpProject\AmpWP\Services;
 
@@ -212,7 +213,8 @@ public function enqueue_assets( $hook_suffix ) {
 		wp_styles()->add_data( self::ASSET_HANDLE, 'rtl', 'replace' );
 
 		$theme           = wp_get_theme();
-		$is_reader_theme = in_array( get_stylesheet(), wp_list_pluck( $this->reader_themes->get_themes(), 'slug' ), true );
+		$reader_theme    = AMP_Options_Manager::get_option( Option::READER_THEME );
+		$is_reader_theme = $this->reader_themes->theme_data_exists( get_stylesheet() );
 
 		$exit_link = menu_page_url( AMP_Options_Manager::OPTION_NAME, false );
 
@@ -238,6 +240,7 @@ public function enqueue_assets( $hook_suffix ) {
 				'screenshot'      => $theme->get_screenshot() ?: null,
 				'url'             => $theme->get( 'ThemeURI' ),
 			],
+			'READER_THEME_AVAILABLE'             => $this->reader_themes->theme_data_exists( $reader_theme ),
 			'FINISH_LINK'                        => $exit_link,
 			'OPTIONS_REST_PATH'                  => '/amp/v1/options',
 			'READER_THEMES_REST_PATH'            => '/amp/v1/reader-themes',

diff --git a/src/Admin/OptionsMenu.php b/src/Admin/OptionsMenu.php
@@ -13,6 +13,7 @@
 use AmpProject\AmpWP\Infrastructure\Conditional;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
+use AmpProject\AmpWP\Option;
 
 /**
  * OptionsMenu class.
@@ -194,7 +195,8 @@ public function enqueue_assets( $hook_suffix ) {
 		wp_styles()->add_data( self::ASSET_HANDLE, 'rtl', 'replace' );
 
 		$theme           = wp_get_theme();
-		$is_reader_theme = in_array( get_stylesheet(), wp_list_pluck( $this->reader_themes->get_themes(), 'slug' ), true );
+		$reader_theme    = AMP_Options_Manager::get_option( Option::READER_THEME );
+		$is_reader_theme = $this->reader_themes->theme_data_exists( get_stylesheet() );
 
 		$js_data = [
 			'CURRENT_THEME'              => [
@@ -212,6 +214,7 @@ public function enqueue_assets( $hook_suffix ) {
 				true
 			),
 			'LEGACY_THEME_SLUG'          => ReaderThemes::DEFAULT_READER_THEME,
+			'READER_THEME_AVAILABLE'     => $this->reader_themes->theme_data_exists( $reader_theme ),
 			'THEME_SUPPORT_ARGS'         => AMP_Theme_Support::get_theme_support_args(),
 			'THEME_SUPPORTS_READER_MODE' => AMP_Theme_Support::supports_reader_mode(),
 			'UPDATES_NONCE'              => wp_create_nonce( 'updates' ),

diff --git a/src/Admin/ReaderThemes.php b/src/Admin/ReaderThemes.php
@@ -336,6 +336,16 @@ public function get_theme_availability( $theme ) {
 		}
 	}
 
+	/**
+	 * Determine if the data for the specified Reader theme exists.
+	 *
+	 * @param string $theme_slug Theme slug.
+	 * @return bool Whether the Reader theme data exists.
+	 */
+	public function theme_data_exists( $theme_slug ) {
+		return in_array( $theme_slug, wp_list_pluck( $this->get_themes(), 'slug' ), true );
+	}
+
 	/**
 	 * Provides details for the legacy theme included with the plugin.
 	 *

diff --git a/src/OptionsRESTController.php b/src/OptionsRESTController.php
@@ -277,7 +277,7 @@ public function get_item_schema() {
 						'arg_options' => [
 							'validate_callback' => function ( $value ) {
 								// Note: The validate_callback is used instead of enum in order to prevent leaking the list of themes.
-								return in_array( $value, wp_list_pluck( $this->reader_themes->get_themes(), 'slug' ), true );
+								return $this->reader_themes->theme_data_exists( $value );
 							},
 						],
 					],

diff --git a/tests/php/src/Admin/ReaderThemesTest.php b/tests/php/src/Admin/ReaderThemesTest.php
@@ -244,4 +244,30 @@ public function test_can_install_theme() {
 		$core_theme['requires_php'] = '999.9';
 		$this->assertFalse( $this->reader_themes->can_install_theme( $core_theme ) );
 	}
+
+	/**
+	 * Tests for theme_data_exists.
+	 *
+	 * @covers ReaderThemes::theme_data_exists
+	 */
+	public function test_theme_data_exists() {
+		$this->assertFalse( ( new ReaderThemes() )->theme_data_exists( 'neve' ) );
+
+		$neve_theme        = [
+			'name'         => 'Neve',
+			'requires'     => false,
+			'requires_php' => '5.2',
+			'slug'         => 'neve',
+		];
+		$append_neve_theme = static function ( $themes ) use ( $neve_theme ) {
+			$themes[] = $neve_theme;
+			return $themes;
+		};
+
+		add_filter( 'amp_reader_themes', $append_neve_theme );
+
+		$this->assertTrue( ( new ReaderThemes() )->theme_data_exists( 'neve' ) );
+
+		remove_filter( 'amp_reader_themes', $append_neve_theme );
+	}
 }