From 5621ad8a5379c23bbd26b4a067027053069fad9a Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 14:54:21 +0300 Subject: [PATCH 01/29] Remove mobile redirect toggle from Onboarding Wizard --- assets/src/onboarding-wizard/pages/summary/reader.js | 3 --- assets/src/onboarding-wizard/pages/summary/transitional.js | 4 ---- tests/e2e/specs/amp-onboarding/summary.js | 3 --- 3 files changed, 10 deletions(-) diff --git a/assets/src/onboarding-wizard/pages/summary/reader.js b/assets/src/onboarding-wizard/pages/summary/reader.js index 864a3a70e77..2324d1399ed 100644 --- a/assets/src/onboarding-wizard/pages/summary/reader.js +++ b/assets/src/onboarding-wizard/pages/summary/reader.js @@ -18,7 +18,6 @@ import { AMPInfo } from '../../../components/amp-info'; import { IconDesktop } from '../../../components/svg/icon-desktop'; import { IconMobile } from '../../../components/svg/icon-mobile'; import { Options } from '../../../components/options-context-provider'; -import { RedirectToggle } from '../../../components/redirect-toggle'; import { ReaderThemes } from '../../../components/reader-themes-context-provider'; import DesktopIcon from '../../../components/svg/desktop-icon.svg'; import MobileIcon from '../../../components/svg/mobile-icon.svg'; @@ -49,8 +48,6 @@ export function Reader( { currentTheme } ) { text={ __( 'In Reader mode your site will have a non-AMP and an AMP version, and each version will use its own theme. If automatic mobile redirection is enabled, the AMP version of the content will be served on mobile devices. If AMP-to-AMP linking is enabled, once users are on an AMP page, they will continue navigating your AMP content.', 'amp' ) } /> - -
diff --git a/assets/src/onboarding-wizard/pages/summary/transitional.js b/assets/src/onboarding-wizard/pages/summary/transitional.js index ef4c81b569d..f802b9a7d79 100644 --- a/assets/src/onboarding-wizard/pages/summary/transitional.js +++ b/assets/src/onboarding-wizard/pages/summary/transitional.js @@ -14,7 +14,6 @@ import { __ } from '@wordpress/i18n'; import { useContext } from '@wordpress/element'; import { Transitional as TransitionalIllustration } from '../../../components/svg/transitional'; import { AMPNotice, NOTICE_TYPE_INFO, NOTICE_SIZE_LARGE } from '../../../components/amp-notice'; -import { RedirectToggle } from '../../../components/redirect-toggle'; import { Options } from '../../../components/options-context-provider'; import { SummaryHeader } from './summary-header'; import { DesktopScreenshot } from './desktop-screenshot'; @@ -42,9 +41,6 @@ export function Transitional( { currentTheme } ) { /> - - - ); } diff --git a/tests/e2e/specs/amp-onboarding/summary.js b/tests/e2e/specs/amp-onboarding/summary.js index 419ab027c68..43f9dad9b67 100644 --- a/tests/e2e/specs/amp-onboarding/summary.js +++ b/tests/e2e/specs/amp-onboarding/summary.js @@ -15,7 +15,6 @@ describe( 'Summary', () => { await expect( page ).toMatchElement( 'h2', { text: 'Standard' } ); await expect( page ).not.toMatchElement( '.phone img' ); - await expect( page ).not.toMatchElement( '.components-form-toggle' ); sharedTests(); } ); @@ -25,7 +24,6 @@ describe( 'Summary', () => { await expect( page ).toMatchElement( 'h2', { text: 'Transitional' } ); await expect( page ).not.toMatchElement( '.phone img' ); - await expect( page ).toMatchElement( '.components-form-toggle.is-checked' ); sharedTests(); } ); @@ -35,7 +33,6 @@ describe( 'Summary', () => { await expect( page ).toMatchElement( 'h2', { text: 'Reader' } ); await expect( page ).toMatchElement( '.phone img' ); - await expect( page ).toMatchElement( '.components-form-toggle.is-checked' ); sharedTests(); } ); From 0a47e5307c3ab9720edf5742e6af64cbbb7b7bfc Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 16:44:12 +0300 Subject: [PATCH 02/29] Add mobile redirection toggle to new Other Settings section --- .../src/components/redirect-toggle/index.js | 26 +++----- .../__snapshots__/redirect-toggle.js.snap | 62 +++++++++---------- assets/src/settings-page/index.js | 16 ++++- .../src/settings-page/mobile-redirection.js | 21 +++---- assets/src/settings-page/style.css | 27 +++++++- 5 files changed, 83 insertions(+), 69 deletions(-) diff --git a/assets/src/components/redirect-toggle/index.js b/assets/src/components/redirect-toggle/index.js index 6b868b5e5bc..ae0c1619db4 100644 --- a/assets/src/components/redirect-toggle/index.js +++ b/assets/src/components/redirect-toggle/index.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import PropTypes from 'prop-types'; - /** * WordPress dependencies */ @@ -15,23 +10,18 @@ import { __ } from '@wordpress/i18n'; import { Options } from '../options-context-provider'; import { AMPSettingToggle } from '../amp-setting-toggle'; -export function RedirectToggle( { direction = 'bottom' } ) { +export function RedirectToggle() { const { editedOptions, updateOptions } = useContext( Options ); const { mobile_redirect: mobileRedirect } = editedOptions; return ( -
- { - updateOptions( { mobile_redirect: ! mobileRedirect } ); - } } - /> -
+ { + updateOptions( { mobile_redirect: ! mobileRedirect } ); + } } + /> ); } -RedirectToggle.propTypes = { - direction: PropTypes.oneOf( [ 'bottom', 'left' ] ), -}; diff --git a/assets/src/components/redirect-toggle/test/__snapshots__/redirect-toggle.js.snap b/assets/src/components/redirect-toggle/test/__snapshots__/redirect-toggle.js.snap index dc497de4dff..5d020faeb40 100644 --- a/assets/src/components/redirect-toggle/test/__snapshots__/redirect-toggle.js.snap +++ b/assets/src/components/redirect-toggle/test/__snapshots__/redirect-toggle.js.snap @@ -2,47 +2,43 @@ exports[`RedirectToggle matches snapshot 1`] = `
-
+ - - - - -
+
diff --git a/assets/src/settings-page/index.js b/assets/src/settings-page/index.js index 04edb93751d..a054ee4de0f 100644 --- a/assets/src/settings-page/index.js +++ b/assets/src/settings-page/index.js @@ -37,11 +37,11 @@ import { ErrorScreen } from '../components/error-screen'; import { Welcome } from './welcome'; import { TemplateModes } from './template-modes'; import { SupportedTemplates } from './supported-templates'; -import { MobileRedirection } from './mobile-redirection'; import { SettingsFooter } from './settings-footer'; import { PluginSuppression } from './plugin-suppression'; import { Analytics } from './analytics'; import { PairedUrlStructure } from './paired-url-structure'; +import { MobileRedirection } from './mobile-redirection'; const { ajaxurl: wpAjaxUrl } = global; @@ -173,7 +173,6 @@ function Root( { appRoot } ) {

{ __( 'Advanced Settings', 'amp' ) }

- + + { __( 'Other', 'amp' ) } + + ) } + hiddenTitle={ __( 'Other', 'amp' ) } + id="other-settings" + initialOpen={ 'other-settings' === focusedSection } + > + + diff --git a/assets/src/settings-page/mobile-redirection.js b/assets/src/settings-page/mobile-redirection.js index 5e315450624..da85c320b73 100644 --- a/assets/src/settings-page/mobile-redirection.js +++ b/assets/src/settings-page/mobile-redirection.js @@ -1,12 +1,8 @@ -/** - * External dependencies - */ -import PropTypes from 'prop-types'; - /** * WordPress dependencies */ import { useContext } from '@wordpress/element'; +import { __ } from '@wordpress/i18n'; /** * Internal dependencies @@ -16,11 +12,8 @@ import { Options } from '../components/options-context-provider'; /** * Mobile redirection section of the settings page. - * - * @param {Object} props Component props. - * @param {string} props.id Unique HTML ID. */ -export function MobileRedirection( { id } ) { +export function MobileRedirection() { const { editedOptions } = useContext( Options ); const { theme_support: themeSupport } = editedOptions || {}; @@ -31,11 +24,11 @@ export function MobileRedirection( { id } ) { } return ( -
- +
+

+ { __( 'Redirects', 'amp' ) } +

+
); } -MobileRedirection.propTypes = { - id: PropTypes.string.isRequired, -}; diff --git a/assets/src/settings-page/style.css b/assets/src/settings-page/style.css index 0b26be8a35d..cd50d3c8fdb 100644 --- a/assets/src/settings-page/style.css +++ b/assets/src/settings-page/style.css @@ -100,7 +100,6 @@ html { font-size: 14px; } -.mobile-redirection, .plugin-suppression { margin-bottom: 1rem; } @@ -360,7 +359,8 @@ li.error-kept { #plugin-suppression .amp-drawer__panel-body-inner, .amp-analytics .amp-drawer__panel-body-inner, -#paired-url-structure .amp-drawer__panel-body-inner { +#paired-url-structure .amp-drawer__panel-body-inner, +.amp-other-settings .amp-drawer__panel-body-inner { padding: 1.5rem 1.5rem 3rem; @media (min-width: 783px) { @@ -529,3 +529,26 @@ li.error-kept { height: 24px; width: 24px; } + +/* Other Settings */ +.amp-other-settings .amp-drawer__panel-body-inner { + padding-top: 0; +} + +.amp-other-settings section + section { + margin-top: 2rem; +} + +.amp-other-settings h4 { + font-size: 1rem; +} + +.amp .amp-other-settings .amp-setting-toggle .components-form-toggle { + margin-right: 1rem; +} + +.amp-other-settings .amp-setting-toggle .amp-setting-toggle__label-text > h3 { + font-size: 0.875rem; + font-weight: bold; + margin: 0; +} From e8b226f092fe75654484252666b1cd7a6e3fba09 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 18:23:03 +0300 Subject: [PATCH 03/29] Move `UserContextProvider` to general components directory --- assets/src/components/unsaved-changes-warning/index.js | 2 +- .../user-context-provider/__mocks__/index.js} | 0 .../user-context-provider/index.js} | 2 +- assets/src/onboarding-wizard/components/nav/index.js | 2 +- assets/src/onboarding-wizard/components/nav/test/index.js | 4 ++-- .../components/template-mode-override-context-provider.js | 2 +- assets/src/onboarding-wizard/index.js | 2 +- assets/src/onboarding-wizard/pages/save/index.js | 2 +- .../src/onboarding-wizard/pages/technical-background/index.js | 2 +- assets/src/onboarding-wizard/pages/template-mode/index.js | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) rename assets/src/{onboarding-wizard/components/__mocks__/user-context-provider.js => components/user-context-provider/__mocks__/index.js} (100%) rename assets/src/{onboarding-wizard/components/user-context-provider.js => components/user-context-provider/index.js} (98%) diff --git a/assets/src/components/unsaved-changes-warning/index.js b/assets/src/components/unsaved-changes-warning/index.js index 6983a0527d9..c83e21570c7 100644 --- a/assets/src/components/unsaved-changes-warning/index.js +++ b/assets/src/components/unsaved-changes-warning/index.js @@ -12,7 +12,7 @@ import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ -import { User } from '../../onboarding-wizard/components/user-context-provider'; +import { User } from '../../components/user-context-provider'; import { Options } from '../options-context-provider'; /** diff --git a/assets/src/onboarding-wizard/components/__mocks__/user-context-provider.js b/assets/src/components/user-context-provider/__mocks__/index.js similarity index 100% rename from assets/src/onboarding-wizard/components/__mocks__/user-context-provider.js rename to assets/src/components/user-context-provider/__mocks__/index.js diff --git a/assets/src/onboarding-wizard/components/user-context-provider.js b/assets/src/components/user-context-provider/index.js similarity index 98% rename from assets/src/onboarding-wizard/components/user-context-provider.js rename to assets/src/components/user-context-provider/index.js index bb0f38b967d..5ee6e287f0e 100644 --- a/assets/src/onboarding-wizard/components/user-context-provider.js +++ b/assets/src/components/user-context-provider/index.js @@ -13,7 +13,7 @@ import PropTypes from 'prop-types'; * Internal dependencies */ import { useAsyncError } from '../../utils/use-async-error'; -import { Options } from '../../components/options-context-provider'; +import { Options } from '../options-context-provider'; export const User = createContext(); diff --git a/assets/src/onboarding-wizard/components/nav/index.js b/assets/src/onboarding-wizard/components/nav/index.js index 3838123c64d..5c2c314bdfe 100644 --- a/assets/src/onboarding-wizard/components/nav/index.js +++ b/assets/src/onboarding-wizard/components/nav/index.js @@ -18,7 +18,7 @@ import { CUSTOMIZER_LINK, AMP_QUERY_VAR } from 'amp-settings'; // From WP inline import { Navigation } from '../navigation-context-provider'; import './style.css'; import { Options } from '../../../components/options-context-provider'; -import { User } from '../user-context-provider'; +import { User } from '../../../components/user-context-provider'; import { READER } from '../../../common/constants'; import { ReaderThemes } from '../../../components/reader-themes-context-provider'; import { useWindowWidth } from '../../../utils/use-window-width'; diff --git a/assets/src/onboarding-wizard/components/nav/test/index.js b/assets/src/onboarding-wizard/components/nav/test/index.js index 9d9c2839ea1..a4ac6d8e687 100644 --- a/assets/src/onboarding-wizard/components/nav/test/index.js +++ b/assets/src/onboarding-wizard/components/nav/test/index.js @@ -15,14 +15,14 @@ import { render } from '@wordpress/element'; */ import { Nav } from '..'; import { NavigationContextProvider } from '../../navigation-context-provider'; -import { UserContextProvider } from '../../user-context-provider'; +import { UserContextProvider } from '../../../../components/user-context-provider'; import { OptionsContextProvider } from '../../../../components/options-context-provider'; import { ReaderThemesContextProvider } from '../../../../components/reader-themes-context-provider'; import { STANDARD, READER } from '../../../../common/constants'; jest.mock( '../../../../components/options-context-provider' ); jest.mock( '../../../../components/reader-themes-context-provider' ); -jest.mock( '../../user-context-provider' ); +jest.mock( '../../../../components/user-context-provider' ); let container; diff --git a/assets/src/onboarding-wizard/components/template-mode-override-context-provider.js b/assets/src/onboarding-wizard/components/template-mode-override-context-provider.js index 8e55e38b99f..c20d3c8fa7e 100644 --- a/assets/src/onboarding-wizard/components/template-mode-override-context-provider.js +++ b/assets/src/onboarding-wizard/components/template-mode-override-context-provider.js @@ -13,8 +13,8 @@ import { createContext, useState, useEffect, useContext } from '@wordpress/eleme */ import { Options } from '../../components/options-context-provider'; import { ReaderThemes } from '../../components/reader-themes-context-provider'; +import { User } from '../../components/user-context-provider'; import { Navigation } from './navigation-context-provider'; -import { User } from './user-context-provider'; export const TemplateModeOverride = createContext(); diff --git a/assets/src/onboarding-wizard/index.js b/assets/src/onboarding-wizard/index.js index f81c38e434b..6753ac7f723 100644 --- a/assets/src/onboarding-wizard/index.js +++ b/assets/src/onboarding-wizard/index.js @@ -33,10 +33,10 @@ import { ReaderThemesContextProvider } from '../components/reader-themes-context import { ErrorBoundary } from '../components/error-boundary'; import { ErrorContextProvider } from '../components/error-context-provider'; import { ErrorScreen } from '../components/error-screen'; +import { UserContextProvider } from '../components/user-context-provider'; import { PAGES } from './pages'; import { SetupWizard } from './setup-wizard'; import { NavigationContextProvider } from './components/navigation-context-provider'; -import { UserContextProvider } from './components/user-context-provider'; import { SiteScanContextProvider } from './components/site-scan-context-provider'; import { TemplateModeOverrideContextProvider } from './components/template-mode-override-context-provider'; diff --git a/assets/src/onboarding-wizard/pages/save/index.js b/assets/src/onboarding-wizard/pages/save/index.js index b875f41f657..c4379d6792d 100644 --- a/assets/src/onboarding-wizard/pages/save/index.js +++ b/assets/src/onboarding-wizard/pages/save/index.js @@ -8,7 +8,7 @@ import { Button } from '@wordpress/components'; /** * Internal dependencies */ -import { User } from '../../components/user-context-provider'; +import { User } from '../../../components/user-context-provider'; import { Phone } from '../../../components/phone'; import './style.css'; import { ReaderThemes } from '../../../components/reader-themes-context-provider'; diff --git a/assets/src/onboarding-wizard/pages/technical-background/index.js b/assets/src/onboarding-wizard/pages/technical-background/index.js index 94839c2161a..289fdcab669 100644 --- a/assets/src/onboarding-wizard/pages/technical-background/index.js +++ b/assets/src/onboarding-wizard/pages/technical-background/index.js @@ -8,7 +8,7 @@ import { useContext, useEffect, useCallback } from '@wordpress/element'; * Internal dependencies */ import { Navigation } from '../../components/navigation-context-provider'; -import { User } from '../../components/user-context-provider'; +import { User } from '../../../components/user-context-provider'; import { User1, User2 } from '../../../components/svg/user-icons'; import { Selectable } from '../../../components/selectable'; import './style.css'; diff --git a/assets/src/onboarding-wizard/pages/template-mode/index.js b/assets/src/onboarding-wizard/pages/template-mode/index.js index ffb6b0f5753..87143108857 100644 --- a/assets/src/onboarding-wizard/pages/template-mode/index.js +++ b/assets/src/onboarding-wizard/pages/template-mode/index.js @@ -8,7 +8,7 @@ import { useEffect, useContext } from '@wordpress/element'; import { Navigation } from '../../components/navigation-context-provider'; import { SiteScan } from '../../components/site-scan-context-provider'; import { ReaderThemes } from '../../../components/reader-themes-context-provider'; -import { User } from '../../components/user-context-provider'; +import { User } from '../../../components/user-context-provider'; import { Options } from '../../../components/options-context-provider'; import { Loading } from '../../../components/loading'; import { TemplateModeOverride } from '../../components/template-mode-override-context-provider'; From 5e77d90710a2d979c178a0176800bc0530a86d84 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 18:24:48 +0300 Subject: [PATCH 04/29] Add `User` context and Dev Tools section to Settings page --- .../src/components/dev-tools-toggle/index.js | 34 +++++++++ .../test/__snapshots__/index.js.snap | 45 ++++++++++++ .../components/dev-tools-toggle/test/index.js | 69 +++++++++++++++++++ assets/src/settings-page/developer-tools.js | 26 +++++++ assets/src/settings-page/index.js | 52 ++++++++++---- assets/src/settings-page/settings-footer.js | 17 +++-- assets/src/settings-page/style.css | 9 +++ src/Admin/OptionsMenu.php | 25 ++++--- 8 files changed, 242 insertions(+), 35 deletions(-) create mode 100644 assets/src/components/dev-tools-toggle/index.js create mode 100644 assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap create mode 100644 assets/src/components/dev-tools-toggle/test/index.js create mode 100644 assets/src/settings-page/developer-tools.js diff --git a/assets/src/components/dev-tools-toggle/index.js b/assets/src/components/dev-tools-toggle/index.js new file mode 100644 index 00000000000..8f1b4fedbf8 --- /dev/null +++ b/assets/src/components/dev-tools-toggle/index.js @@ -0,0 +1,34 @@ +/** + * WordPress dependencies + */ +import { useContext } from '@wordpress/element'; +import { __ } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import { AMPSettingToggle } from '../amp-setting-toggle'; +import { User } from '../../components/user-context-provider'; +import { Loading } from '../loading'; + +export function DevToolsToggle() { + const { + developerToolsOption, + fetchingUser, + setDeveloperToolsOption, + } = useContext( User ); + + if ( fetchingUser ) { + return ; + } + + return ( + { + setDeveloperToolsOption( ! developerToolsOption ); + } } + /> + ); +} diff --git a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap new file mode 100644 index 00000000000..548ea1c53ee --- /dev/null +++ b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap @@ -0,0 +1,45 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`DevToolsToggle matches snapshot 1`] = ` +
+
+
+ + + + + + +
+
+
+`; diff --git a/assets/src/components/dev-tools-toggle/test/index.js b/assets/src/components/dev-tools-toggle/test/index.js new file mode 100644 index 00000000000..25fcd9387d7 --- /dev/null +++ b/assets/src/components/dev-tools-toggle/test/index.js @@ -0,0 +1,69 @@ +/** + * External dependencies + */ +import { act } from 'react-dom/test-utils'; +import { create } from 'react-test-renderer'; + +/** + * WordPress dependencies + */ +import { render } from '@wordpress/element'; + +/** + * Internal dependencies + */ +import { DevToolsToggle } from '../'; +import { UserContextProvider } from '../../user-context-provider'; + +jest.mock( '../../user-context-provider' ); + +let container; + +describe( 'DevToolsToggle', () => { + beforeEach( () => { + container = document.createElement( 'div' ); + document.body.appendChild( container ); + } ); + + afterEach( () => { + document.body.removeChild( container ); + container = null; + } ); + + it( 'matches snapshot', () => { + const wrapper = create( + + + , + ); + expect( wrapper.toJSON() ).toMatchSnapshot(); + } ); + + it( 'can be toggled', () => { + act( () => { + render( + + + , + container, + ); + } ); + expect( container.querySelector( 'input:checked' ) ).toBeNull(); + + act( + () => { + container.querySelector( 'input' ).dispatchEvent( new global.MouseEvent( 'click' ) ); + }, + ); + + expect( container.querySelector( 'input:checked' ) ).not.toBeNull(); + + act( + () => { + container.querySelector( 'input' ).dispatchEvent( new global.MouseEvent( 'click' ) ); + }, + ); + + expect( container.querySelector( 'input:checked' ) ).toBeNull(); + } ); +} ); diff --git a/assets/src/settings-page/developer-tools.js b/assets/src/settings-page/developer-tools.js new file mode 100644 index 00000000000..13d0667a805 --- /dev/null +++ b/assets/src/settings-page/developer-tools.js @@ -0,0 +1,26 @@ +/** + * WordPress dependencies + */ +import { __ } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import { DevToolsToggle } from '../components/dev-tools-toggle'; + +/** + * Developer tools section of the settings page. + */ +export function DeveloperTools() { + return ( +
+

+ { __( 'Dev Tools', 'amp' ) } +

+

+ { __( 'This will only be enabled for your user account. This is not a sitewide setting.', 'amp' ) } +

+ +
+ ); +} diff --git a/assets/src/settings-page/index.js b/assets/src/settings-page/index.js index a054ee4de0f..9329e434d31 100644 --- a/assets/src/settings-page/index.js +++ b/assets/src/settings-page/index.js @@ -8,13 +8,15 @@ import { OPTIONS_REST_PATH, READER_THEMES_REST_PATH, UPDATES_NONCE, + USER_FIELD_DEVELOPER_TOOLS_ENABLED, + USER_REST_PATH, } from 'amp-settings'; /** * WordPress dependencies */ import domReady from '@wordpress/dom-ready'; -import { render, useContext, useState, useEffect } from '@wordpress/element'; +import { render, useContext, useState, useEffect, useCallback } from '@wordpress/element'; import { __ } from '@wordpress/i18n'; /** @@ -34,6 +36,7 @@ import { ErrorContextProvider } from '../components/error-context-provider'; import { AMPDrawer } from '../components/amp-drawer'; import { AMPNotice, NOTICE_SIZE_LARGE } from '../components/amp-notice'; import { ErrorScreen } from '../components/error-screen'; +import { User, UserContextProvider } from '../components/user-context-provider'; import { Welcome } from './welcome'; import { TemplateModes } from './template-modes'; import { SupportedTemplates } from './supported-templates'; @@ -42,6 +45,7 @@ import { PluginSuppression } from './plugin-suppression'; import { Analytics } from './analytics'; import { PairedUrlStructure } from './paired-url-structure'; import { MobileRedirection } from './mobile-redirection'; +import { DeveloperTools } from './developer-tools'; const { ajaxurl: wpAjaxUrl } = global; @@ -65,16 +69,21 @@ function Providers( { children } ) { optionsRestPath={ OPTIONS_REST_PATH } populateDefaultValues={ true } > - - { children } - + + { children } + + @@ -117,9 +126,24 @@ function scrollFocusedSectionIntoView( focusedSectionId ) { function Root( { appRoot } ) { const [ focusedSection, setFocusedSection ] = useState( global.location.hash.replace( /^#/, '' ) ); - const { fetchingOptions, saveOptions } = useContext( Options ); + const { hasOptionsChanges, fetchingOptions, saveOptions } = useContext( Options ); + const { hasDeveloperToolsOptionChange, saveDeveloperToolsOption } = useContext( User ); const { templateModeWasOverridden } = useContext( ReaderThemes ); + /** + * Handle the form submit event. + */ + const onSubmit = useCallback( ( event ) => { + event.preventDefault(); + + if ( hasOptionsChanges ) { + saveOptions(); + } + if ( hasDeveloperToolsOptionChange ) { + saveDeveloperToolsOption(); + } + }, [ hasDeveloperToolsOptionChange, hasOptionsChanges, saveDeveloperToolsOption, saveOptions ] ); + /** * Scroll to the focused element on load or when it changes. */ @@ -165,10 +189,7 @@ function Root( { appRoot } ) { ) } -
{ - event.preventDefault(); - saveOptions(); - } }> +

{ __( 'Advanced Settings', 'amp' ) } @@ -224,6 +245,7 @@ function Root( { appRoot } ) { initialOpen={ 'other-settings' === focusedSection } > + diff --git a/assets/src/settings-page/settings-footer.js b/assets/src/settings-page/settings-footer.js index 534275bfa51..51a4e237771 100644 --- a/assets/src/settings-page/settings-footer.js +++ b/assets/src/settings-page/settings-footer.js @@ -20,6 +20,7 @@ import { AMPNotice, NOTICE_TYPE_ERROR, NOTICE_TYPE_SUCCESS } from '../components import { Options } from '../components/options-context-provider'; import { ReaderThemes } from '../components/reader-themes-context-provider'; import { ErrorContext } from '../components/error-context-provider'; +import { User } from '../components/user-context-provider'; /** * Renders an error notice. @@ -53,27 +54,25 @@ export function SettingsFooter() { const { didSaveOptions, editedOptions, hasOptionsChanges, savingOptions } = useContext( Options ); const { downloadingTheme } = useContext( ReaderThemes ); const { error } = useContext( ErrorContext ); + const { didSaveDeveloperToolsOption, hasDeveloperToolsOptionChange, savingDeveloperToolsOption } = useContext( User ); const { reader_theme: readerTheme, theme_support: themeSupport } = editedOptions; - const disabled = ! themeSupport || - ! hasOptionsChanges || - savingOptions || - didSaveOptions || - downloadingTheme || - ( 'reader' === themeSupport && ! readerTheme ); + const hasChanges = hasOptionsChanges || hasDeveloperToolsOptionChange; + const isBusy = savingOptions || downloadingTheme || savingDeveloperToolsOption; + const disabled = ! hasChanges || isBusy || ! themeSupport || ( 'reader' === themeSupport && ! readerTheme ); return (
- { error && } - { ! error && ! hasOptionsChanges && didSaveOptions && ! downloadingTheme && ( + { ! error && ! hasChanges && ! downloadingTheme && ( didSaveOptions || didSaveDeveloperToolsOption ) && ( reader_themes->theme_data_exists( get_stylesheet() ); $js_data = [ - 'AMP_QUERY_VAR' => amp_get_slug(), - 'CURRENT_THEME' => [ + 'AMP_QUERY_VAR' => amp_get_slug(), + 'CURRENT_THEME' => [ 'name' => $theme->get( 'Name' ), 'description' => $theme->get( 'Description' ), 'is_reader_theme' => $is_reader_theme, 'screenshot' => $theme->get_screenshot() ?: null, 'url' => $theme->get( 'ThemeURI' ), ], - 'HAS_DEPENDENCY_SUPPORT' => $this->dependency_support->has_support(), - 'OPTIONS_REST_PATH' => '/amp/v1/options', - 'READER_THEMES_REST_PATH' => '/amp/v1/reader-themes', - 'IS_CORE_THEME' => in_array( + 'HAS_DEPENDENCY_SUPPORT' => $this->dependency_support->has_support(), + 'OPTIONS_REST_PATH' => '/amp/v1/options', + 'READER_THEMES_REST_PATH' => '/amp/v1/reader-themes', + 'IS_CORE_THEME' => in_array( get_stylesheet(), AMP_Core_Theme_Sanitizer::get_supported_themes(), true ), - 'LEGACY_THEME_SLUG' => ReaderThemes::DEFAULT_READER_THEME, - 'USING_FALLBACK_READER_THEME' => $this->reader_themes->using_fallback_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' ), + 'LEGACY_THEME_SLUG' => ReaderThemes::DEFAULT_READER_THEME, + 'USING_FALLBACK_READER_THEME' => $this->reader_themes->using_fallback_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' ), + 'USER_FIELD_DEVELOPER_TOOLS_ENABLED' => UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, + 'USER_REST_PATH' => '/wp/v2/users/me', ]; wp_add_inline_script( From 07b7697c14a96e764554448ab7679ac78086df7f Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 18:47:41 +0300 Subject: [PATCH 05/29] Fix e2e test --- tests/e2e/utils/onboarding-wizard-utils.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tests/e2e/utils/onboarding-wizard-utils.js b/tests/e2e/utils/onboarding-wizard-utils.js index 68801f9eb05..f35e098673a 100644 --- a/tests/e2e/utils/onboarding-wizard-utils.js +++ b/tests/e2e/utils/onboarding-wizard-utils.js @@ -84,25 +84,6 @@ export async function moveToSummaryScreen( { technical = true, mode, readerTheme export async function moveToDoneScreen( { technical = true, mode, readerTheme = 'legacy', mobileRedirect = true } ) { await moveToSummaryScreen( { technical, mode, readerTheme, mobileRedirect } ); - if ( 'standard' !== mode ) { - await page.waitForSelector( '.amp-setting-toggle input' ); - - const selector = '.amp-setting-toggle .components-form-toggle.is-checked'; - const checkedMobileRedirect = await page.waitForSelector( selector ); - - if ( checkedMobileRedirect && false === mobileRedirect ) { - const labelSelector = `${ selector } + label`; - await page.evaluate( ( selectorLabel ) => { - document.querySelector( selectorLabel ).scrollIntoView(); - }, labelSelector ); - await expect( page ).toClick( labelSelector ); - await page.waitForSelector( '.amp-setting-toggle .components-form-toggle:not(.is-checked)' ); - } else if ( ! checkedMobileRedirect && true === mobileRedirect ) { - await expect( page ).toClick( selector ); - await page.waitForSelector( selector ); - } - } - await clickNextButton(); await page.waitForTimeout( 1000 ); await page.waitForSelector( '.done__preview-container' ); From a1dfcd097ffd4e7e7580bc81ee7531a7f4954137 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 19:04:55 +0300 Subject: [PATCH 06/29] Open "Other" panel before searching for mobile redirect toggle --- tests/e2e/specs/admin/mobile-redirect.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/e2e/specs/admin/mobile-redirect.js b/tests/e2e/specs/admin/mobile-redirect.js index 4b331477743..d5eac320ba2 100644 --- a/tests/e2e/specs/admin/mobile-redirect.js +++ b/tests/e2e/specs/admin/mobile-redirect.js @@ -8,6 +8,7 @@ const { visitAdminPage } = require( '@wordpress/e2e-test-utils/build/visit-admin */ const { completeWizard, cleanUpSettings } = require( '../../utils/onboarding-wizard-utils' ); +const panelToggleSelector = '#other-settings .components-panel__body-toggle'; const toggleSelector = '.mobile-redirection .amp-setting-toggle input[type="checkbox"]'; describe( 'Mobile redirect settings', () => { @@ -19,6 +20,7 @@ describe( 'Mobile redirect settings', () => { await completeWizard( { mode: 'reader', mobileRedirect: true } ); await visitAdminPage( 'admin.php', 'page=amp-options' ); + await expect( page ).toClick( panelToggleSelector ); await page.waitForSelector( toggleSelector ); await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); } ); @@ -27,6 +29,7 @@ describe( 'Mobile redirect settings', () => { await completeWizard( { mode: 'reader', mobileRedirect: false } ); await visitAdminPage( 'admin.php', 'page=amp-options' ); + await expect( page ).toClick( panelToggleSelector ); await page.waitForSelector( toggleSelector ); await expect( page ).not.toMatchElement( `${ toggleSelector }:checked` ); } ); From 041ba12d80f354d54ac963f0cae744cb11b41072 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 30 Jul 2021 19:17:27 +0300 Subject: [PATCH 07/29] Wait for "Other" panel before attempting to click it --- tests/e2e/specs/admin/mobile-redirect.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/e2e/specs/admin/mobile-redirect.js b/tests/e2e/specs/admin/mobile-redirect.js index d5eac320ba2..47d5cef4441 100644 --- a/tests/e2e/specs/admin/mobile-redirect.js +++ b/tests/e2e/specs/admin/mobile-redirect.js @@ -20,6 +20,7 @@ describe( 'Mobile redirect settings', () => { await completeWizard( { mode: 'reader', mobileRedirect: true } ); await visitAdminPage( 'admin.php', 'page=amp-options' ); + await page.waitForSelector( panelToggleSelector ); await expect( page ).toClick( panelToggleSelector ); await page.waitForSelector( toggleSelector ); await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); @@ -29,6 +30,7 @@ describe( 'Mobile redirect settings', () => { await completeWizard( { mode: 'reader', mobileRedirect: false } ); await visitAdminPage( 'admin.php', 'page=amp-options' ); + await page.waitForSelector( panelToggleSelector ); await expect( page ).toClick( panelToggleSelector ); await page.waitForSelector( toggleSelector ); await expect( page ).not.toMatchElement( `${ toggleSelector }:checked` ); From 490dfc9eab840c07fda21ad69bea7468fdfadc72 Mon Sep 17 00:00:00 2001 From: Joshua Wold Date: Fri, 30 Jul 2021 13:25:47 -0700 Subject: [PATCH 08/29] Updated headings within the Other section of Settings Per the comment from Weston on https://github.com/ampproject/amp-wp/issues/5578#issuecomment-890089314, I'm suggesting a change to the new "Other" panel. The goal is that the design should now look like this: https://d.pr/i/CPtRce. Feedback is welcome, haven't submitted a PR in a while! --- assets/src/settings-page/developer-tools.js | 7 ++----- assets/src/settings-page/mobile-redirection.js | 3 --- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/assets/src/settings-page/developer-tools.js b/assets/src/settings-page/developer-tools.js index 13d0667a805..cdf87a94919 100644 --- a/assets/src/settings-page/developer-tools.js +++ b/assets/src/settings-page/developer-tools.js @@ -14,13 +14,10 @@ import { DevToolsToggle } from '../components/dev-tools-toggle'; export function DeveloperTools() { return (
-

- { __( 'Dev Tools', 'amp' ) } -

+

- { __( 'This will only be enabled for your user account. This is not a sitewide setting.', 'amp' ) } + { __( 'Only enabled for your user account. This is not a sitewide setting.', 'amp' ) }

-
); } diff --git a/assets/src/settings-page/mobile-redirection.js b/assets/src/settings-page/mobile-redirection.js index da85c320b73..1b1c403c94c 100644 --- a/assets/src/settings-page/mobile-redirection.js +++ b/assets/src/settings-page/mobile-redirection.js @@ -25,9 +25,6 @@ export function MobileRedirection() { return (
-

- { __( 'Redirects', 'amp' ) } -

); From 87fc3b10a6279bc8066097401e729c39bb5beac5 Mon Sep 17 00:00:00 2001 From: Joshua Wold Date: Fri, 30 Jul 2021 13:31:34 -0700 Subject: [PATCH 09/29] Update developer-tools.js Saw an error to properly indent. Trying again. --- assets/src/settings-page/developer-tools.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/settings-page/developer-tools.js b/assets/src/settings-page/developer-tools.js index cdf87a94919..6fb1b1c563c 100644 --- a/assets/src/settings-page/developer-tools.js +++ b/assets/src/settings-page/developer-tools.js @@ -16,7 +16,7 @@ export function DeveloperTools() {

- { __( 'Only enabled for your user account. This is not a sitewide setting.', 'amp' ) } + { __( 'Only enabled for your user account. This is not a sitewide setting.', 'amp' ) }

); From 051619825768e19fded96edbccaee2eef5d4c990 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 16 Aug 2021 14:04:10 +0200 Subject: [PATCH 10/29] Remove unused import --- assets/src/settings-page/mobile-redirection.js | 1 - 1 file changed, 1 deletion(-) diff --git a/assets/src/settings-page/mobile-redirection.js b/assets/src/settings-page/mobile-redirection.js index 1b1c403c94c..5d064d804b7 100644 --- a/assets/src/settings-page/mobile-redirection.js +++ b/assets/src/settings-page/mobile-redirection.js @@ -2,7 +2,6 @@ * WordPress dependencies */ import { useContext } from '@wordpress/element'; -import { __ } from '@wordpress/i18n'; /** * Internal dependencies From f32516ab1753b53a3599b6a15af8fc38d925882b Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 16 Aug 2021 14:14:28 +0200 Subject: [PATCH 11/29] Update snapshots --- .../dev-tools-toggle/test/__snapshots__/index.js.snap | 4 ++-- .../test/__snapshots__/redirect-toggle.js.snap | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap index 548ea1c53ee..9033c0334d8 100644 --- a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap +++ b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap @@ -5,10 +5,10 @@ exports[`DevToolsToggle matches snapshot 1`] = ` className="amp-setting-toggle " >
Date: Mon, 16 Aug 2021 14:52:22 +0200 Subject: [PATCH 12/29] Add unit test for loading state --- .../test/__snapshots__/index.js.snap | 10 +++++++++ .../components/dev-tools-toggle/test/index.js | 21 +++++++++++++++++++ .../user-context-provider/__mocks__/index.js | 9 +++++--- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap index 9033c0334d8..070c70619ac 100644 --- a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap +++ b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap @@ -43,3 +43,13 @@ exports[`DevToolsToggle matches snapshot 1`] = `
`; + +exports[`DevToolsToggle matches snapshot for the loading state 1`] = ` +
+ +
+`; diff --git a/assets/src/components/dev-tools-toggle/test/index.js b/assets/src/components/dev-tools-toggle/test/index.js index 25fcd9387d7..960bee9893b 100644 --- a/assets/src/components/dev-tools-toggle/test/index.js +++ b/assets/src/components/dev-tools-toggle/test/index.js @@ -39,6 +39,27 @@ describe( 'DevToolsToggle', () => { expect( wrapper.toJSON() ).toMatchSnapshot(); } ); + it( 'renders a loading spinner when a user data is fetched', () => { + act( () => { + render( + + + , + container, + ); + } ); + expect( container.querySelector( '.amp-spinner-container' ) ).not.toBeNull(); + } ); + + it( 'matches snapshot for the loading state', () => { + const wrapper = create( + + + , + ); + expect( wrapper.toJSON() ).toMatchSnapshot(); + } ); + it( 'can be toggled', () => { act( () => { render( diff --git a/assets/src/components/user-context-provider/__mocks__/index.js b/assets/src/components/user-context-provider/__mocks__/index.js index d6ca7ab5ee6..2d096ae54b4 100644 --- a/assets/src/components/user-context-provider/__mocks__/index.js +++ b/assets/src/components/user-context-provider/__mocks__/index.js @@ -13,14 +13,16 @@ export const User = createContext(); /** * MOCK. * - * @param {Object} props - * @param {any} props.children + * @param {Object} props + * @param {any} props.children + * @param {boolean} props.fetchingUser */ -export function UserContextProvider( { children } ) { +export function UserContextProvider( { children, fetchingUser } ) { return ( { children } @@ -29,4 +31,5 @@ export function UserContextProvider( { children } ) { } UserContextProvider.propTypes = { children: PropTypes.any, + fetchingUser: PropTypes.bool, }; From 06833b63f0f7b22da31844f2ae6a6cf088fc494a Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 16 Aug 2021 19:00:57 +0200 Subject: [PATCH 13/29] Fix Other settings box spacing --- assets/src/settings-page/style.css | 4 ---- 1 file changed, 4 deletions(-) diff --git a/assets/src/settings-page/style.css b/assets/src/settings-page/style.css index 5551157f015..1b19d20709e 100644 --- a/assets/src/settings-page/style.css +++ b/assets/src/settings-page/style.css @@ -531,10 +531,6 @@ li.error-kept { } /* Other Settings */ -.amp-other-settings .amp-drawer__panel-body-inner { - padding-top: 0; -} - .amp-other-settings section + section { margin-top: 2rem; } From 9948aa74cd12add489ceecf09b175c65d37bf235 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 16 Aug 2021 20:18:40 +0200 Subject: [PATCH 14/29] Fix E2E test --- tests/e2e/specs/admin/mobile-redirect.js | 36 ++++++++++++---------- tests/e2e/utils/onboarding-wizard-utils.js | 8 ++--- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/tests/e2e/specs/admin/mobile-redirect.js b/tests/e2e/specs/admin/mobile-redirect.js index 47d5cef4441..74c28597fe8 100644 --- a/tests/e2e/specs/admin/mobile-redirect.js +++ b/tests/e2e/specs/admin/mobile-redirect.js @@ -6,33 +6,37 @@ const { visitAdminPage } = require( '@wordpress/e2e-test-utils/build/visit-admin /** * Internal dependencies */ -const { completeWizard, cleanUpSettings } = require( '../../utils/onboarding-wizard-utils' ); +const { cleanUpSettings, scrollToElement } = require( '../../utils/onboarding-wizard-utils' ); -const panelToggleSelector = '#other-settings .components-panel__body-toggle'; -const toggleSelector = '.mobile-redirection .amp-setting-toggle input[type="checkbox"]'; +const panelSelector = '#other-settings .components-panel__body-toggle'; +const toggleSelector = '#other-settings .mobile-redirection .amp-setting-toggle input[type="checkbox"]'; describe( 'Mobile redirect settings', () => { - afterEach( async () => { + it( 'persists the mobile redirect setting value', async () => { + // Disable mobile redirection by calling the `cleanUpSettings` function. await cleanUpSettings(); - } ); - - it( 'persists the mobile redirect setting on', async () => { - await completeWizard( { mode: 'reader', mobileRedirect: true } ); await visitAdminPage( 'admin.php', 'page=amp-options' ); - await page.waitForSelector( panelToggleSelector ); - await expect( page ).toClick( panelToggleSelector ); + // Confirm the mobile redirection is disabled. + await page.waitForSelector( panelSelector ); + await scrollToElement( { selector: panelSelector, click: true } ); + await page.waitForSelector( toggleSelector ); + await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); + + // Disable the setting and save. await page.waitForSelector( toggleSelector ); + await scrollToElement( { selector: toggleSelector, click: true } ); await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); - } ); + await scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ); - it( 'persists the mobile redirect setting off', async () => { - await completeWizard( { mode: 'reader', mobileRedirect: false } ); + // Refresh the page once the settings have been saved. + await page.waitForTimeout( 1000 ); await visitAdminPage( 'admin.php', 'page=amp-options' ); - await page.waitForSelector( panelToggleSelector ); - await expect( page ).toClick( panelToggleSelector ); + // Confirm the mobile redirection setting has been persisted. + await page.waitForSelector( panelSelector ); + await scrollToElement( { selector: panelSelector, click: true } ); await page.waitForSelector( toggleSelector ); - await expect( page ).not.toMatchElement( `${ toggleSelector }:checked` ); + await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); } ); } ); diff --git a/tests/e2e/utils/onboarding-wizard-utils.js b/tests/e2e/utils/onboarding-wizard-utils.js index f35e098673a..c8bd87d8615 100644 --- a/tests/e2e/utils/onboarding-wizard-utils.js +++ b/tests/e2e/utils/onboarding-wizard-utils.js @@ -81,16 +81,16 @@ export async function moveToSummaryScreen( { technical = true, mode, readerTheme await page.waitForSelector( '.summary' ); } -export async function moveToDoneScreen( { technical = true, mode, readerTheme = 'legacy', mobileRedirect = true } ) { - await moveToSummaryScreen( { technical, mode, readerTheme, mobileRedirect } ); +export async function moveToDoneScreen( { technical = true, mode, readerTheme = 'legacy' } ) { + await moveToSummaryScreen( { technical, mode, readerTheme } ); await clickNextButton(); await page.waitForTimeout( 1000 ); await page.waitForSelector( '.done__preview-container' ); } -export async function completeWizard( { technical = true, mode, readerTheme = 'legacy', mobileRedirect = true } ) { - await moveToDoneScreen( { technical, mode, readerTheme, mobileRedirect } ); +export async function completeWizard( { technical = true, mode, readerTheme = 'legacy' } ) { + await moveToDoneScreen( { technical, mode, readerTheme } ); if ( 'reader' === mode ) { await visitAdminPage( 'admin.php', 'page=amp-options' ); } else { From f51a83d2e976ffd57f2bd8a502fc43d5c4dbba16 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 16 Aug 2021 20:30:00 +0200 Subject: [PATCH 15/29] Add E2E test for dev tools toggle --- tests/e2e/specs/admin/mobile-redirect.js | 42 -------------- tests/e2e/specs/admin/other-settings.js | 73 ++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 42 deletions(-) delete mode 100644 tests/e2e/specs/admin/mobile-redirect.js create mode 100644 tests/e2e/specs/admin/other-settings.js diff --git a/tests/e2e/specs/admin/mobile-redirect.js b/tests/e2e/specs/admin/mobile-redirect.js deleted file mode 100644 index 74c28597fe8..00000000000 --- a/tests/e2e/specs/admin/mobile-redirect.js +++ /dev/null @@ -1,42 +0,0 @@ -/** - * WordPress dependencies - */ -const { visitAdminPage } = require( '@wordpress/e2e-test-utils/build/visit-admin-page' ); - -/** - * Internal dependencies - */ -const { cleanUpSettings, scrollToElement } = require( '../../utils/onboarding-wizard-utils' ); - -const panelSelector = '#other-settings .components-panel__body-toggle'; -const toggleSelector = '#other-settings .mobile-redirection .amp-setting-toggle input[type="checkbox"]'; - -describe( 'Mobile redirect settings', () => { - it( 'persists the mobile redirect setting value', async () => { - // Disable mobile redirection by calling the `cleanUpSettings` function. - await cleanUpSettings(); - await visitAdminPage( 'admin.php', 'page=amp-options' ); - - // Confirm the mobile redirection is disabled. - await page.waitForSelector( panelSelector ); - await scrollToElement( { selector: panelSelector, click: true } ); - await page.waitForSelector( toggleSelector ); - await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); - - // Disable the setting and save. - await page.waitForSelector( toggleSelector ); - await scrollToElement( { selector: toggleSelector, click: true } ); - await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); - await scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ); - - // Refresh the page once the settings have been saved. - await page.waitForTimeout( 1000 ); - await visitAdminPage( 'admin.php', 'page=amp-options' ); - - // Confirm the mobile redirection setting has been persisted. - await page.waitForSelector( panelSelector ); - await scrollToElement( { selector: panelSelector, click: true } ); - await page.waitForSelector( toggleSelector ); - await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); - } ); -} ); diff --git a/tests/e2e/specs/admin/other-settings.js b/tests/e2e/specs/admin/other-settings.js new file mode 100644 index 00000000000..f3a93efbf62 --- /dev/null +++ b/tests/e2e/specs/admin/other-settings.js @@ -0,0 +1,73 @@ +/** + * WordPress dependencies + */ +const { visitAdminPage } = require( '@wordpress/e2e-test-utils/build/visit-admin-page' ); + +/** + * Internal dependencies + */ +const { cleanUpSettings, scrollToElement, completeWizard } = require( '../../utils/onboarding-wizard-utils' ); + +const panelSelector = '#other-settings .components-panel__body-toggle'; + +describe( 'Other settings', () => { + beforeEach( async () => { + await completeWizard( { mode: 'transitional' } ); + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + await page.waitForSelector( panelSelector ); + await scrollToElement( { selector: panelSelector, click: true } ); + } ); + + afterEach( async () => { + await cleanUpSettings(); + } ); + + it( 'persists the mobile redirect setting value', async () => { + const toggleSelector = '#other-settings .mobile-redirection .amp-setting-toggle input[type="checkbox"]'; + + // Confirm the mobile redirection is initially disabled. + await page.waitForSelector( toggleSelector ); + await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); + + // Disable the setting and save. + await page.waitForSelector( toggleSelector ); + await scrollToElement( { selector: toggleSelector, click: true } ); + await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); + await scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ); + + // Refresh the page once the settings have been saved. + await page.waitForTimeout( 1000 ); + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + // Confirm the mobile redirection setting is persisted. + await page.waitForSelector( panelSelector ); + await scrollToElement( { selector: panelSelector, click: true } ); + await page.waitForSelector( toggleSelector ); + await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); + } ); + + it( 'persists the dev tools setting value', async () => { + const toggleSelector = '#other-settings .developer-tools .amp-setting-toggle input[type="checkbox"]'; + + // Confirm the dev tools setting is enabled initially. + await page.waitForSelector( toggleSelector ); + await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); + + // Disable the setting and save. + await page.waitForSelector( toggleSelector ); + await scrollToElement( { selector: toggleSelector, click: true } ); + await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); + await scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ); + + // Refresh the page once the settings have been saved. + await page.waitForTimeout( 1000 ); + await visitAdminPage( 'admin.php', 'page=amp-options' ); + + // Confirm the dev tools setting is persisted. + await page.waitForSelector( panelSelector ); + await scrollToElement( { selector: panelSelector, click: true } ); + await page.waitForSelector( toggleSelector ); + await expect( page ).toMatchElement( `${ toggleSelector }:not(:checked)` ); + } ); +} ); From bcd1e67527d20620d5a86fe22ecb4df08bba2f4e Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 19 Aug 2021 17:50:34 +0200 Subject: [PATCH 16/29] Apply suggestions from code review Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com> --- assets/src/components/dev-tools-toggle/index.js | 4 ++-- assets/src/components/unsaved-changes-warning/index.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/assets/src/components/dev-tools-toggle/index.js b/assets/src/components/dev-tools-toggle/index.js index 8f1b4fedbf8..deac840fa16 100644 --- a/assets/src/components/dev-tools-toggle/index.js +++ b/assets/src/components/dev-tools-toggle/index.js @@ -8,7 +8,7 @@ import { __ } from '@wordpress/i18n'; * Internal dependencies */ import { AMPSettingToggle } from '../amp-setting-toggle'; -import { User } from '../../components/user-context-provider'; +import { User } from '../user-context-provider'; import { Loading } from '../loading'; export function DevToolsToggle() { @@ -25,7 +25,7 @@ export function DevToolsToggle() { return ( { setDeveloperToolsOption( ! developerToolsOption ); } } diff --git a/assets/src/components/unsaved-changes-warning/index.js b/assets/src/components/unsaved-changes-warning/index.js index c83e21570c7..820fb7cb382 100644 --- a/assets/src/components/unsaved-changes-warning/index.js +++ b/assets/src/components/unsaved-changes-warning/index.js @@ -12,7 +12,7 @@ import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ -import { User } from '../../components/user-context-provider'; +import { User } from '../user-context-provider'; import { Options } from '../options-context-provider'; /** From a7c7cf1f108e050013802b82888dcdef8a75b587 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Thu, 19 Aug 2021 18:06:49 +0200 Subject: [PATCH 17/29] Update Jest snapshot --- .../dev-tools-toggle/test/__snapshots__/index.js.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap index 070c70619ac..ef447da2e34 100644 --- a/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap +++ b/assets/src/components/dev-tools-toggle/test/__snapshots__/index.js.snap @@ -35,7 +35,7 @@ exports[`DevToolsToggle matches snapshot 1`] = ` className="amp-setting-toggle__label-text" >

- Enable Dev Tools + Enable Developer Tools

From a1f9b31e80bdee2b9f22ffcd30d4bb2035595d1b Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 20 Aug 2021 14:56:02 +0200 Subject: [PATCH 18/29] Improve dev tools toggle description --- assets/src/settings-page/developer-tools.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/assets/src/settings-page/developer-tools.js b/assets/src/settings-page/developer-tools.js index 6fb1b1c563c..acf309b05fb 100644 --- a/assets/src/settings-page/developer-tools.js +++ b/assets/src/settings-page/developer-tools.js @@ -16,7 +16,10 @@ export function DeveloperTools() {

- { __( 'Only enabled for your user account. This is not a sitewide setting.', 'amp' ) } + { __( 'Enable AMP developer tools to surface validation errors when editing posts and viewing the site.', 'amp' ) } +

+

+ { __( 'Only enabled for your user account. This is not a sitewide setting. This presumes you have some experience coding with HTML, CSS, JS, and PHP.', 'amp' ) }

); From 2f7d0b55f0302ed915e6e2f5af89c51ed72f6401 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 3 Sep 2021 11:29:53 +0200 Subject: [PATCH 19/29] Allow user-related changes on Options page --- .../components/user-context-provider/index.js | 17 ++++++++++------- assets/src/onboarding-wizard/index.js | 1 + src/Admin/OptionsMenu.php | 1 + 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/assets/src/components/user-context-provider/index.js b/assets/src/components/user-context-provider/index.js index 5ee6e287f0e..57cf166786b 100644 --- a/assets/src/components/user-context-provider/index.js +++ b/assets/src/components/user-context-provider/index.js @@ -20,13 +20,15 @@ export const User = createContext(); /** * Context provider user data. * - * @param {Object} props Component props. - * @param {?any} props.children Component children. - * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. - * @param {string} props.userRestPath REST endpoint to retrieve options. + * @param {Object} props Component props. + * @param {?any} props.children Component children. + * @param {boolean} props.allowConfiguredPluginOnly Provide only if the plugin has been configured (Onboarding Wizard complete). + * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. + * @param {string} props.userRestPath REST endpoint to retrieve options. */ -export function UserContextProvider( { children, userOptionDeveloperTools, userRestPath } ) { +export function UserContextProvider( { children, allowConfiguredPluginOnly = false, userOptionDeveloperTools, userRestPath } ) { const { originalOptions, fetchingOptions } = useContext( Options ); + const { plugin_configured: pluginConfigured } = originalOptions; const [ fetchingUser, setFetchingUser ] = useState( false ); const [ developerToolsOption, setDeveloperToolsOption ] = useState( null ); const [ originalDeveloperToolsOption, setOriginalDeveloperToolsOption ] = useState( null ); @@ -52,7 +54,7 @@ export function UserContextProvider( { children, userOptionDeveloperTools, userR return; } - if ( ! originalOptions.plugin_configured ) { + if ( ! pluginConfigured && allowConfiguredPluginOnly ) { setOriginalDeveloperToolsOption( null ); setDeveloperToolsOption( null ); return; @@ -84,7 +86,7 @@ export function UserContextProvider( { children, userOptionDeveloperTools, userR setFetchingUser( false ); } )(); - }, [ fetchingOptions, fetchingUser, originalDeveloperToolsOption, originalOptions.plugin_configured, setAsyncError, userOptionDeveloperTools, userRestPath ] ); + }, [ allowConfiguredPluginOnly, fetchingOptions, fetchingUser, originalDeveloperToolsOption, pluginConfigured, setAsyncError, userOptionDeveloperTools, userRestPath ] ); /** * Sends the option back to the REST endpoint to be saved. @@ -136,6 +138,7 @@ export function UserContextProvider( { children, userOptionDeveloperTools, userR UserContextProvider.propTypes = { children: PropTypes.any, + allowConfiguredPluginOnly: PropTypes.bool, userOptionDeveloperTools: PropTypes.string.isRequired, userRestPath: PropTypes.string.isRequired, }; diff --git a/assets/src/onboarding-wizard/index.js b/assets/src/onboarding-wizard/index.js index 6753ac7f723..e5b03996c8f 100644 --- a/assets/src/onboarding-wizard/index.js +++ b/assets/src/onboarding-wizard/index.js @@ -67,6 +67,7 @@ export function Providers( { children } ) { populateDefaultValues={ false } > diff --git a/src/Admin/OptionsMenu.php b/src/Admin/OptionsMenu.php index 0fd24138247..c0fe71fe156 100644 --- a/src/Admin/OptionsMenu.php +++ b/src/Admin/OptionsMenu.php @@ -302,6 +302,7 @@ protected function add_preload_rest_paths() { '/amp/v1/options', '/amp/v1/reader-themes', '/wp/v2/settings', + '/wp/v2/users/me', ]; foreach ( $paths as $path ) { From ca6a642256454a212b160cbbae9fafe28562bcb1 Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Fri, 3 Sep 2021 15:36:51 -0500 Subject: [PATCH 20/29] Fix typo --- tests/e2e/specs/admin/other-settings.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/specs/admin/other-settings.js b/tests/e2e/specs/admin/other-settings.js index f3a93efbf62..21e6dd8263f 100644 --- a/tests/e2e/specs/admin/other-settings.js +++ b/tests/e2e/specs/admin/other-settings.js @@ -26,7 +26,7 @@ describe( 'Other settings', () => { it( 'persists the mobile redirect setting value', async () => { const toggleSelector = '#other-settings .mobile-redirection .amp-setting-toggle input[type="checkbox"]'; - // Confirm the mobile redirection is initially disabled. + // Confirm the mobile redirection is initially enabled. await page.waitForSelector( toggleSelector ); await expect( page ).toMatchElement( `${ toggleSelector }:checked` ); From bbadaf217304205c8456663315f251bc6ae0d6eb Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Fri, 3 Sep 2021 15:37:14 -0500 Subject: [PATCH 21/29] Update param description --- assets/src/components/user-context-provider/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/components/user-context-provider/index.js b/assets/src/components/user-context-provider/index.js index 57cf166786b..21f632565c6 100644 --- a/assets/src/components/user-context-provider/index.js +++ b/assets/src/components/user-context-provider/index.js @@ -22,7 +22,7 @@ export const User = createContext(); * * @param {Object} props Component props. * @param {?any} props.children Component children. - * @param {boolean} props.allowConfiguredPluginOnly Provide only if the plugin has been configured (Onboarding Wizard complete). + * @param {boolean} props.allowConfiguredPluginOnly Provided only for functionality that requires the plugin to be configured. * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. * @param {string} props.userRestPath REST endpoint to retrieve options. */ From 121ca24f93e741dd4a4149efb3e0e4bbf93351b4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 16:46:53 -0700 Subject: [PATCH 22/29] Add missing tests for OptionsMenu --- tests/php/src/Admin/OptionsMenuTest.php | 91 ++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/tests/php/src/Admin/OptionsMenuTest.php b/tests/php/src/Admin/OptionsMenuTest.php index 0936c543b52..94d93a5849a 100644 --- a/tests/php/src/Admin/OptionsMenuTest.php +++ b/tests/php/src/Admin/OptionsMenuTest.php @@ -17,6 +17,7 @@ use AmpProject\AmpWP\Infrastructure\Service; use AmpProject\AmpWP\LoadingError; use AmpProject\AmpWP\Tests\TestCase; +use AMP_Options_Manager; /** * Tests for OptionsMenu. @@ -34,7 +35,7 @@ class OptionsMenuTest extends TestCase { public $instance; /** - * Setup. + * Set up. * * @inheritdoc */ @@ -43,6 +44,30 @@ public function setUp() { $this->instance = new OptionsMenu( new GoogleFonts(), new ReaderThemes(), new RESTPreloader(), new DependencySupport(), new LoadingError() ); } + /** + * Tear down. + * + * @inheritdoc + */ + public function tearDown() { + parent::tearDown(); + $GLOBALS['wp_scripts'] = null; + $GLOBALS['wp_styles'] = null; + } + + /** @covers ::is_needed() */ + public function test_is_needed() { + $this->assertFalse( is_admin() ); + set_current_screen( 'index.php' ); + $this->assertTrue( OptionsMenu::is_needed() ); + + add_filter( 'amp_options_menu_is_enabled', '__return_false' ); + $this->assertFalse( OptionsMenu::is_needed() ); + + add_filter( 'amp_options_menu_is_enabled', '__return_true', 20 ); + $this->assertTrue( OptionsMenu::is_needed() ); + } + /** @covers ::__construct() */ public function test__construct() { $this->assertInstanceOf( OptionsMenu::class, $this->instance ); @@ -64,6 +89,7 @@ public function test_constants() { * Test add_hooks. * * @see OptionsMenu::add_hooks() + * @cogers ::register() */ public function test_register() { $this->instance->register(); @@ -74,6 +100,26 @@ public function test_register() { $this->assertEquals( 10, has_action( 'admin_enqueue_scripts', [ $this->instance, 'enqueue_assets' ] ) ); } + /** @covers ::add_plugin_action_links() */ + public function test_add_plugin_action_links() { + $links = [ + 'example' => 'example!', + ]; + + $filtered_links = $this->instance->add_plugin_action_links( $links ); + + $this->assertArrayHasKey( 'example', $filtered_links ); + $this->assertArrayHasKey( 'settings', $filtered_links ); + } + + /** @covers ::get_menu_slug() */ + public function test_get_menu_slug() { + $this->assertSame( + AMP_Options_Manager::OPTION_NAME, + $this->instance->get_menu_slug() + ); + } + /** * Test admin_menu. * @@ -105,6 +151,49 @@ public function test_add_menu_items() { $_parent_pages = $original_parent_pages; } + /** @covers ::screen_handle() */ + public function test_screen_handle() { + $this->assertSame( + 'toplevel_page_' . $this->instance->get_menu_slug(), + $this->instance->screen_handle() + ); + } + + /** @covers ::enqueue_assets() */ + public function test_enqueue_assets_wrong_hook_suffix() { + $this->instance->enqueue_assets( 'nope' ); + $this->assertEquals( 0, did_action( 'amp_register_polyfills' ) ); + $this->assertFalse( wp_script_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + $this->assertFalse( wp_style_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + } + + /** @covers ::enqueue_assets() */ + public function test_enqueue_assets_right_hook_suffix() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->assertFalse( wp_script_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + $this->assertFalse( wp_style_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + + add_action( 'admin_enqueue_scripts', [ $this->instance, 'enqueue_assets' ] ); + do_action( 'admin_enqueue_scripts', $this->instance->screen_handle() ); + $this->assertEquals( 1, did_action( 'amp_register_polyfills' ) ); + + $this->assertTrue( wp_script_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + $this->assertTrue( wp_style_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); + + $script_before = implode( "\n", wp_scripts()->get_data( OptionsMenu::ASSET_HANDLE, 'before' ) ); + $this->assertStringContainsString( 'var ampSettings', $script_before ); + $this->assertStringContainsString( 'USER_FIELD_DEVELOPER_TOOLS_ENABLED', $script_before ); + $this->assertStringContainsString( 'USER_REST_PATH', $script_before ); + + $wp_api_fetch_after = implode( "\n", wp_scripts()->get_data( 'wp-api-fetch', 'after' ) ); + if ( function_exists( 'rest_preload_api_request' ) ) { + $this->assertStringContainsString( wp_json_encode( '/amp/v1/options' ), $wp_api_fetch_after ); + $this->assertStringContainsString( wp_json_encode( '/amp/v1/reader-themes' ), $wp_api_fetch_after ); + $this->assertStringContainsString( wp_json_encode( '/wp/v2/settings' ), $wp_api_fetch_after ); + $this->assertStringContainsString( wp_json_encode( '/wp/v2/users/me' ), $wp_api_fetch_after ); + } + } + /** * Test render_screen for admin users. * From ba02cab7c5ea6c44e8e84d457865d034c9b1fc80 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 17:05:15 -0700 Subject: [PATCH 23/29] Enable mobile redirection by default --- src/MobileRedirection.php | 2 +- tests/php/src/MobileRedirectionTest.php | 34 ++++++++++++-------- tests/php/test-class-amp-options-manager.php | 2 +- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/MobileRedirection.php b/src/MobileRedirection.php index 5836a9b0331..7a9eb183883 100644 --- a/src/MobileRedirection.php +++ b/src/MobileRedirection.php @@ -88,7 +88,7 @@ public function register() { * @return array Defaults. */ public function filter_default_options( $defaults ) { - $defaults[ Option::MOBILE_REDIRECT ] = false; + $defaults[ Option::MOBILE_REDIRECT ] = true; return $defaults; } diff --git a/tests/php/src/MobileRedirectionTest.php b/tests/php/src/MobileRedirectionTest.php index 756dbe4982c..6e635ed6a10 100644 --- a/tests/php/src/MobileRedirectionTest.php +++ b/tests/php/src/MobileRedirectionTest.php @@ -90,36 +90,42 @@ public function test_register_transitional_mode() { /** @covers ::register() */ public function test_register_not_enabled() { + remove_all_filters( 'amp_to_amp_linking_enabled' ); AMP_Options_Manager::update_option( Option::MOBILE_REDIRECT, false ); - $this->instance->register(); - $this->assertSame( 10, has_filter( 'amp_default_options', [ $this->instance, 'filter_default_options' ] ) ); - $this->assertSame( 10, has_filter( 'amp_options_updating', [ $this->instance, 'sanitize_options' ] ) ); - $this->assert_hooks_not_added(); + $instance = new MobileRedirection( $this->paired_routing ); + $instance->register(); + $this->assertSame( 10, has_filter( 'amp_default_options', [ $instance, 'filter_default_options' ] ) ); + $this->assertSame( 10, has_filter( 'amp_options_updating', [ $instance, 'sanitize_options' ] ) ); + $this->assert_hooks_not_added( $instance ); } /** @covers ::register() */ public function test_register_enabled_but_standard_mode() { + remove_all_filters( 'amp_to_amp_linking_enabled' ); AMP_Options_Manager::update_options( [ Option::MOBILE_REDIRECT => true, Option::THEME_SUPPORT => AMP_Theme_Support::STANDARD_MODE_SLUG, ] ); - $this->instance->register(); - $this->assertSame( 10, has_filter( 'amp_default_options', [ $this->instance, 'filter_default_options' ] ) ); - $this->assertSame( 10, has_filter( 'amp_options_updating', [ $this->instance, 'sanitize_options' ] ) ); - $this->assert_hooks_not_added(); + $instance = new MobileRedirection( $this->paired_routing ); + $instance->register(); + $this->assertSame( 10, has_filter( 'amp_default_options', [ $instance, 'filter_default_options' ] ) ); + $this->assertSame( 10, has_filter( 'amp_options_updating', [ $instance, 'sanitize_options' ] ) ); + $this->assert_hooks_not_added( $instance ); } /** * Assert the service hooks were not added. + * + * @param MobileRedirection $instance */ - private function assert_hooks_not_added() { - $this->assertFalse( has_action( 'template_redirect', [ $this->instance, 'redirect' ] ) ); + private function assert_hooks_not_added( MobileRedirection $instance ) { + $this->assertFalse( has_action( 'template_redirect', [ $instance, 'redirect' ] ) ); $this->assertFalse( has_filter( 'amp_to_amp_linking_enabled', '__return_true' ) ); - $this->assertFalse( has_filter( 'comment_post_redirect', [ $this->instance, 'filter_comment_post_redirect' ] ) ); - $this->assertFalse( has_filter( 'get_comments_link', [ $this->instance, 'add_noamp_mobile_query_var' ] ) ); - $this->assertFalse( has_filter( 'respond_link', [ $this->instance, 'add_noamp_mobile_query_var' ] ) ); + $this->assertFalse( has_filter( 'comment_post_redirect', [ $instance, 'filter_comment_post_redirect' ] ) ); + $this->assertFalse( has_filter( 'get_comments_link', [ $instance, 'add_noamp_mobile_query_var' ] ) ); + $this->assertFalse( has_filter( 'respond_link', [ $instance, 'add_noamp_mobile_query_var' ] ) ); } /** @covers ::filter_default_options() */ @@ -128,7 +134,7 @@ public function test_filter_default_options() { $this->assertEquals( [ 'foo' => 'bar', - Option::MOBILE_REDIRECT => false, + Option::MOBILE_REDIRECT => true, ], $this->instance->filter_default_options( [ 'foo' => 'bar' ] ) ); diff --git a/tests/php/test-class-amp-options-manager.php b/tests/php/test-class-amp-options-manager.php index afe62f37036..96a52b3bb50 100644 --- a/tests/php/test-class-amp-options-manager.php +++ b/tests/php/test-class-amp-options-manager.php @@ -114,7 +114,7 @@ public function test_get_and_set_options() { Option::SUPPORTED_TEMPLATES => [ 'is_singular' ], Option::SUPPRESSED_PLUGINS => [], Option::VERSION => AMP__VERSION, - Option::MOBILE_REDIRECT => false, + Option::MOBILE_REDIRECT => true, Option::READER_THEME => 'legacy', Option::PLUGIN_CONFIGURED => false, Option::PAIRED_URL_STRUCTURE => Option::PAIRED_URL_STRUCTURE_QUERY_VAR, From 7162a052a10425434bc4ac6ba1532f46530b6342 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 9 Sep 2021 17:10:24 -0700 Subject: [PATCH 24/29] Set current screen to fix test_enqueue_assets_right_hook_suffix --- tests/php/src/Admin/OptionsMenuTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/php/src/Admin/OptionsMenuTest.php b/tests/php/src/Admin/OptionsMenuTest.php index 94d93a5849a..de7d541ae84 100644 --- a/tests/php/src/Admin/OptionsMenuTest.php +++ b/tests/php/src/Admin/OptionsMenuTest.php @@ -170,6 +170,8 @@ public function test_enqueue_assets_wrong_hook_suffix() { /** @covers ::enqueue_assets() */ public function test_enqueue_assets_right_hook_suffix() { wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + set_current_screen( $this->instance->screen_handle() ); + $this->assertFalse( wp_script_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); $this->assertFalse( wp_style_is( OptionsMenu::ASSET_HANDLE, 'enqueued' ) ); From aafb4b1d98d15a10e5ac51ed0ba969f77af5e287 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 10 Sep 2021 14:03:07 +0200 Subject: [PATCH 25/29] Update assets/src/settings-page/developer-tools.js Co-authored-by: Weston Ruter --- assets/src/settings-page/developer-tools.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/settings-page/developer-tools.js b/assets/src/settings-page/developer-tools.js index acf309b05fb..11e9a026629 100644 --- a/assets/src/settings-page/developer-tools.js +++ b/assets/src/settings-page/developer-tools.js @@ -19,7 +19,7 @@ export function DeveloperTools() { { __( 'Enable AMP developer tools to surface validation errors when editing posts and viewing the site.', 'amp' ) }

- { __( 'Only enabled for your user account. This is not a sitewide setting. This presumes you have some experience coding with HTML, CSS, JS, and PHP.', 'amp' ) } + { __( 'This is a per-user setting. It presumes you have some experience coding with HTML, CSS, JS, and PHP. Once enabled you will have access to Validated URLs and Error Index in the admin menu, the Validate URL item in the admin bar, and the AMP Validation sidebar in the editor.', 'amp' ) }

); From 418ec3c9785cd7c263275c485eb87365d173ad1a Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 10 Sep 2021 14:26:22 +0200 Subject: [PATCH 26/29] Update `users` resource prop and its description --- .../components/user-context-provider/index.js | 24 ++++++++++++------- .../__mocks__/amp-settings.js | 2 +- assets/src/onboarding-wizard/index.js | 4 ++-- assets/src/settings-page/index.js | 4 ++-- src/Admin/OnboardingWizardSubmenuPage.php | 2 +- src/Admin/OptionsMenu.php | 2 +- tests/php/src/Admin/OptionsMenuTest.php | 4 ++-- 7 files changed, 25 insertions(+), 17 deletions(-) diff --git a/assets/src/components/user-context-provider/index.js b/assets/src/components/user-context-provider/index.js index 21f632565c6..253f2e13473 100644 --- a/assets/src/components/user-context-provider/index.js +++ b/assets/src/components/user-context-provider/index.js @@ -24,9 +24,9 @@ export const User = createContext(); * @param {?any} props.children Component children. * @param {boolean} props.allowConfiguredPluginOnly Provided only for functionality that requires the plugin to be configured. * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. - * @param {string} props.userRestPath REST endpoint to retrieve options. + * @param {string} props.usersResourceRestPath The REST path for interacting with the `users` resources. */ -export function UserContextProvider( { children, allowConfiguredPluginOnly = false, userOptionDeveloperTools, userRestPath } ) { +export function UserContextProvider( { children, allowConfiguredPluginOnly = false, userOptionDeveloperTools, usersResourceRestPath } ) { const { originalOptions, fetchingOptions } = useContext( Options ); const { plugin_configured: pluginConfigured } = originalOptions; const [ fetchingUser, setFetchingUser ] = useState( false ); @@ -60,7 +60,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal return; } - if ( ! userRestPath || fetchingUser || null !== originalDeveloperToolsOption ) { + if ( ! usersResourceRestPath || fetchingUser || null !== originalDeveloperToolsOption ) { return; } @@ -71,7 +71,9 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal setFetchingUser( true ); try { - const fetchedUser = await apiFetch( { path: userRestPath } ); + const fetchedUser = await apiFetch( { + path: `${ usersResourceRestPath }/me`, + } ); if ( true === hasUnmounted.current ) { return; @@ -86,7 +88,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal setFetchingUser( false ); } )(); - }, [ allowConfiguredPluginOnly, fetchingOptions, fetchingUser, originalDeveloperToolsOption, pluginConfigured, setAsyncError, userOptionDeveloperTools, userRestPath ] ); + }, [ allowConfiguredPluginOnly, fetchingOptions, fetchingUser, originalDeveloperToolsOption, pluginConfigured, setAsyncError, userOptionDeveloperTools, usersResourceRestPath ] ); /** * Sends the option back to the REST endpoint to be saved. @@ -99,7 +101,13 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal setSavingDeveloperToolsOption( true ); try { - const fetchedUser = await apiFetch( { method: 'post', path: userRestPath, data: { [ userOptionDeveloperTools ]: developerToolsOption } } ); + const fetchedUser = await apiFetch( { + method: 'post', + path: `${ usersResourceRestPath }/me`, + data: { + [ userOptionDeveloperTools ]: developerToolsOption, + }, + } ); if ( true === hasUnmounted.current ) { return; @@ -114,7 +122,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal setDidSaveDeveloperToolsOption( true ); setSavingDeveloperToolsOption( false ); - }, [ hasDeveloperToolsOptionChange, developerToolsOption, setAsyncError, userOptionDeveloperTools, userRestPath ] ); + }, [ hasDeveloperToolsOptionChange, developerToolsOption, setAsyncError, userOptionDeveloperTools, usersResourceRestPath ] ); return ( '/amp/v1/reader-themes', 'UPDATES_NONCE' => wp_create_nonce( 'updates' ), 'USER_FIELD_DEVELOPER_TOOLS_ENABLED' => UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, - 'USER_REST_PATH' => '/wp/v2/users/me', + 'USERS_RESOURCE_REST_PATH' => '/wp/v2/users', ]; wp_add_inline_script( diff --git a/src/Admin/OptionsMenu.php b/src/Admin/OptionsMenu.php index c0fe71fe156..513ff873f74 100644 --- a/src/Admin/OptionsMenu.php +++ b/src/Admin/OptionsMenu.php @@ -245,7 +245,7 @@ public function enqueue_assets( $hook_suffix ) { 'THEME_SUPPORTS_READER_MODE' => AMP_Theme_Support::supports_reader_mode(), 'UPDATES_NONCE' => wp_create_nonce( 'updates' ), 'USER_FIELD_DEVELOPER_TOOLS_ENABLED' => UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, - 'USER_REST_PATH' => '/wp/v2/users/me', + 'USERS_RESOURCE_REST_PATH' => '/wp/v2/users', ]; wp_add_inline_script( diff --git a/tests/php/src/Admin/OptionsMenuTest.php b/tests/php/src/Admin/OptionsMenuTest.php index de7d541ae84..2225f865851 100644 --- a/tests/php/src/Admin/OptionsMenuTest.php +++ b/tests/php/src/Admin/OptionsMenuTest.php @@ -185,14 +185,14 @@ public function test_enqueue_assets_right_hook_suffix() { $script_before = implode( "\n", wp_scripts()->get_data( OptionsMenu::ASSET_HANDLE, 'before' ) ); $this->assertStringContainsString( 'var ampSettings', $script_before ); $this->assertStringContainsString( 'USER_FIELD_DEVELOPER_TOOLS_ENABLED', $script_before ); - $this->assertStringContainsString( 'USER_REST_PATH', $script_before ); + $this->assertStringContainsString( 'USERS_RESOURCE_REST_PATH', $script_before ); $wp_api_fetch_after = implode( "\n", wp_scripts()->get_data( 'wp-api-fetch', 'after' ) ); if ( function_exists( 'rest_preload_api_request' ) ) { $this->assertStringContainsString( wp_json_encode( '/amp/v1/options' ), $wp_api_fetch_after ); $this->assertStringContainsString( wp_json_encode( '/amp/v1/reader-themes' ), $wp_api_fetch_after ); $this->assertStringContainsString( wp_json_encode( '/wp/v2/settings' ), $wp_api_fetch_after ); - $this->assertStringContainsString( wp_json_encode( '/wp/v2/users/me' ), $wp_api_fetch_after ); + $this->assertStringContainsString( wp_json_encode( '/wp/v2/users' ), $wp_api_fetch_after ); } } From ca79001570c879940db713e0d4fccb98f9686b07 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 10 Sep 2021 14:33:31 +0200 Subject: [PATCH 27/29] Fix the unit test --- tests/php/src/Admin/OptionsMenuTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/src/Admin/OptionsMenuTest.php b/tests/php/src/Admin/OptionsMenuTest.php index 2225f865851..be5a35b704a 100644 --- a/tests/php/src/Admin/OptionsMenuTest.php +++ b/tests/php/src/Admin/OptionsMenuTest.php @@ -192,7 +192,7 @@ public function test_enqueue_assets_right_hook_suffix() { $this->assertStringContainsString( wp_json_encode( '/amp/v1/options' ), $wp_api_fetch_after ); $this->assertStringContainsString( wp_json_encode( '/amp/v1/reader-themes' ), $wp_api_fetch_after ); $this->assertStringContainsString( wp_json_encode( '/wp/v2/settings' ), $wp_api_fetch_after ); - $this->assertStringContainsString( wp_json_encode( '/wp/v2/users' ), $wp_api_fetch_after ); + $this->assertStringContainsString( wp_json_encode( '/wp/v2/users/me' ), $wp_api_fetch_after ); } } From acfab6fdf69bd52e002980e51cb83dee71ef50f7 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Fri, 10 Sep 2021 15:42:01 +0200 Subject: [PATCH 28/29] Try fixing failing E2E test --- tests/e2e/specs/admin/amp-options.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/e2e/specs/admin/amp-options.js b/tests/e2e/specs/admin/amp-options.js index 0aaee6963af..1afe9af87b5 100644 --- a/tests/e2e/specs/admin/amp-options.js +++ b/tests/e2e/specs/admin/amp-options.js @@ -113,6 +113,7 @@ describe( 'Saving', () => { it( 'allows saving', async () => { const testSave = async () => { await expect( page ).toClick( 'button', { text: 'Save' } ); + await page.waitForSelector( 'button[disabled].is-busy' ); await expect( page ).toMatchElement( 'button[disabled].is-busy', { text: 'Saving' } ); await expect( page ).toMatchElement( 'button[disabled]', { text: 'Save' } ); await expect( page ).toMatchElement( '.amp-save-success-notice', { text: 'Saved' } ); @@ -121,7 +122,7 @@ describe( 'Saving', () => { // Save button exists. await expect( page ).toMatchElement( 'button[disabled]', { text: 'Save' } ); - // Toggle transistional mode. + // Toggle transitional mode. await expect( page ).toClick( '#template-mode-transitional' ); // Button should be enabled. From 4b3a89a66faaaa46f825c21fea3eb00813b423e2 Mon Sep 17 00:00:00 2001 From: Piotr Delawski Date: Mon, 13 Sep 2021 13:38:42 +0200 Subject: [PATCH 29/29] Improve prop name and description --- .../components/user-context-provider/index.js | 19 ++++++++++--------- assets/src/onboarding-wizard/index.js | 1 - assets/src/settings-page/index.js | 1 + 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/assets/src/components/user-context-provider/index.js b/assets/src/components/user-context-provider/index.js index 253f2e13473..5e6ccaa6495 100644 --- a/assets/src/components/user-context-provider/index.js +++ b/assets/src/components/user-context-provider/index.js @@ -20,13 +20,13 @@ export const User = createContext(); /** * Context provider user data. * - * @param {Object} props Component props. - * @param {?any} props.children Component children. - * @param {boolean} props.allowConfiguredPluginOnly Provided only for functionality that requires the plugin to be configured. - * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. - * @param {string} props.usersResourceRestPath The REST path for interacting with the `users` resources. + * @param {Object} props Component props. + * @param {?any} props.children Component children. + * @param {boolean} props.onlyFetchIfPluginIsConfigured Flag indicating whether the users data should be fetched only if the plugin is fully configured (i.e. the Onboarding Wizard has been completed). + * @param {string} props.userOptionDeveloperTools The key of the option to use from the settings endpoint. + * @param {string} props.usersResourceRestPath The REST path for interacting with the `users` resources. */ -export function UserContextProvider( { children, allowConfiguredPluginOnly = false, userOptionDeveloperTools, usersResourceRestPath } ) { +export function UserContextProvider( { children, onlyFetchIfPluginIsConfigured = true, userOptionDeveloperTools, usersResourceRestPath } ) { const { originalOptions, fetchingOptions } = useContext( Options ); const { plugin_configured: pluginConfigured } = originalOptions; const [ fetchingUser, setFetchingUser ] = useState( false ); @@ -46,6 +46,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal () => null !== developerToolsOption && developerToolsOption !== originalDeveloperToolsOption, [ developerToolsOption, originalDeveloperToolsOption ], ); + /** * Fetch user options on mount. */ @@ -54,7 +55,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal return; } - if ( ! pluginConfigured && allowConfiguredPluginOnly ) { + if ( ! pluginConfigured && onlyFetchIfPluginIsConfigured ) { setOriginalDeveloperToolsOption( null ); setDeveloperToolsOption( null ); return; @@ -88,7 +89,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal setFetchingUser( false ); } )(); - }, [ allowConfiguredPluginOnly, fetchingOptions, fetchingUser, originalDeveloperToolsOption, pluginConfigured, setAsyncError, userOptionDeveloperTools, usersResourceRestPath ] ); + }, [ onlyFetchIfPluginIsConfigured, fetchingOptions, fetchingUser, originalDeveloperToolsOption, pluginConfigured, setAsyncError, userOptionDeveloperTools, usersResourceRestPath ] ); /** * Sends the option back to the REST endpoint to be saved. @@ -146,7 +147,7 @@ export function UserContextProvider( { children, allowConfiguredPluginOnly = fal UserContextProvider.propTypes = { children: PropTypes.any, - allowConfiguredPluginOnly: PropTypes.bool, + onlyFetchIfPluginIsConfigured: PropTypes.bool, userOptionDeveloperTools: PropTypes.string.isRequired, usersResourceRestPath: PropTypes.string.isRequired, }; diff --git a/assets/src/onboarding-wizard/index.js b/assets/src/onboarding-wizard/index.js index 01e1033fb7d..ea7db292c5e 100644 --- a/assets/src/onboarding-wizard/index.js +++ b/assets/src/onboarding-wizard/index.js @@ -67,7 +67,6 @@ export function Providers( { children } ) { populateDefaultValues={ false } > diff --git a/assets/src/settings-page/index.js b/assets/src/settings-page/index.js index 56cd13ee874..c3fb9826251 100644 --- a/assets/src/settings-page/index.js +++ b/assets/src/settings-page/index.js @@ -70,6 +70,7 @@ function Providers( { children } ) { populateDefaultValues={ true } >