-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do async site scan on plugin activation instead of synchronous validation #6685
Merged
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
3a5a93c
Discontinue doing synchronous validation requests upon activating plu…
delawski 80deda6
Do async Site Scan in admin notice after activating plugin
delawski be44ec1
Suppress `stylelint` check around usage of `dashicons` font
delawski 8148d25
Add missing phpdoc and use more accurate method names
delawski 68a6214
Ensure scannable URLs are fetched before starting site scan
delawski 555af01
Keep post-plugin-activation site scan logic in single component
delawski 68f68c2
Hide details for non-technical users
delawski 17bedd5
Add link to Plugin Suppression panel
delawski 3857c7a
Extract `PluginActivationSiteScan` code to separate class
delawski a8738a9
Retrieve source slug and URL on which validation error was found
delawski 0622bf6
Add class name to `AdminNotice`
delawski 5a2e770
Display plugins causing AMP issues as separate component
delawski 756dddf
Add styles to admin notice site scan component
delawski d0070f3
Display AMP URLs if not in Standard template mode
delawski 0f720e7
Add E2E tests for automatic Site Scan after plugin activation
delawski 4eb2054
Add JS docblock
delawski 5253a32
Use SPAN for loading spinner wrapper if inline element is requested
delawski 0929c4a
Merge branch 'develop' of github.com:ampproject/amp-wp into feature/a…
westonruter 87e4fef
Fix phpcs alignment warning
westonruter 80f6efc
Apply notice text update suggestions from code review
delawski ba2ddbc
Cleanly skip test if dependency is missing
delawski 0f2c38a
Rename `AdminNotice` component to `AmpAdminNotice`
delawski a5e649f
Always pass 2 arguments to `visitAdminPage()`
delawski bbf33af
Merge branch 'develop' into feature/async-scan-on-plugin-activation
delawski 0954bd4
Update E2E tests to reflect recent changes
delawski 3a0d9be
Update plugin activation message if validation errors are found
delawski 985e882
Ensure demo plugin is deactivated before running tests
delawski 9bd4c44
Improve plugin activation post-site-scan message copy
delawski e0b2a1c
Update E2E tests
delawski 18178b1
Ensure timeout has been set before trying to clear it
delawski f6f5ff5
Enqueue missing `wp-components` CSS dependency
delawski b17b93a
Do not scan site for network activated plugins
delawski f2a0937
Cover case when Gutenberg is not active with E2E tests
delawski 5b9a3cd
Merge branch 'develop' of github.com:ampproject/amp-wp into feature/a…
westonruter 473fdbd
Show URLs with validation issues to non-technical users as well
westonruter 6e8191c
Open validated URLs in new window to prevent accidental loss
westonruter e23dfa9
Add noopener to external link
westonruter 6c15d77
Harden is_needed check to include has_cap
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import PropTypes from 'prop-types'; | ||
import { | ||
APP_ROOT_ID, | ||
OPTIONS_REST_PATH, | ||
SCANNABLE_URLS_REST_PATH, | ||
USER_FIELD_DEVELOPER_TOOLS_ENABLED, | ||
USERS_RESOURCE_REST_PATH, | ||
VALIDATE_NONCE, | ||
} from 'amp-site-scan-notice'; // From WP inline script. | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import domReady from '@wordpress/dom-ready'; | ||
import { render } from '@wordpress/element'; | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import './style.scss'; | ||
import { ErrorContextProvider } from '../../components/error-context-provider'; | ||
import { OptionsContextProvider } from '../../components/options-context-provider'; | ||
import { PluginsContextProvider } from '../../components/plugins-context-provider'; | ||
import { SiteScanContextProvider } from '../../components/site-scan-context-provider'; | ||
import { UserContextProvider } from '../../components/user-context-provider'; | ||
import { ErrorScreen } from '../../components/error-screen'; | ||
import { ErrorBoundary } from '../../components/error-boundary'; | ||
import { SiteScanNotice } from './notice'; | ||
|
||
let errorHandler; | ||
|
||
/** | ||
* Context providers for the application. | ||
* | ||
* @param {Object} props Component props. | ||
* @param {any} props.children Component children. | ||
*/ | ||
function Providers( { children } ) { | ||
global.removeEventListener( 'error', errorHandler ); | ||
|
||
return ( | ||
<ErrorContextProvider> | ||
<ErrorBoundary | ||
title={ __( 'The AMP Site Scanner has experienced an error.', 'amp' ) } | ||
> | ||
<OptionsContextProvider | ||
hasErrorBoundary={ true } | ||
optionsRestPath={ OPTIONS_REST_PATH } | ||
populateDefaultValues={ false } | ||
> | ||
<UserContextProvider | ||
onlyFetchIfPluginIsConfigured={ true } | ||
userOptionDeveloperTools={ USER_FIELD_DEVELOPER_TOOLS_ENABLED } | ||
usersResourceRestPath={ USERS_RESOURCE_REST_PATH } | ||
> | ||
<PluginsContextProvider hasErrorBoundary={ true }> | ||
<SiteScanContextProvider | ||
scannableUrlsRestPath={ SCANNABLE_URLS_REST_PATH } | ||
validateNonce={ VALIDATE_NONCE } | ||
> | ||
{ children } | ||
</SiteScanContextProvider> | ||
</PluginsContextProvider> | ||
</UserContextProvider> | ||
</OptionsContextProvider> | ||
</ErrorBoundary> | ||
</ErrorContextProvider> | ||
); | ||
} | ||
Providers.propTypes = { | ||
children: PropTypes.any, | ||
}; | ||
|
||
domReady( () => { | ||
const root = document.getElementById( APP_ROOT_ID ); | ||
|
||
if ( ! root ) { | ||
return; | ||
} | ||
|
||
errorHandler = ( event ) => { | ||
// Handle only own errors. | ||
if ( event.filename && /amp-site-scan-notice(\.min)?\.js/.test( event.filename ) ) { | ||
render( <ErrorScreen error={ event.error } />, root ); | ||
} | ||
}; | ||
|
||
global.addEventListener( 'error', errorHandler ); | ||
|
||
render( | ||
<Providers> | ||
<SiteScanNotice /> | ||
</Providers>, | ||
root, | ||
); | ||
} ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useContext, useEffect } from '@wordpress/element'; | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* External dependencies | ||
*/ | ||
import { | ||
AMP_COMPATIBLE_PLUGINS_URL, | ||
SETTINGS_LINK, | ||
} from 'amp-site-scan-notice'; // From WP inline script. | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { SiteScan } from '../../components/site-scan-context-provider'; | ||
import { | ||
AMP_ADMIN_NOTICE_TYPE_ERROR, | ||
AMP_ADMIN_NOTICE_TYPE_INFO, | ||
AMP_ADMIN_NOTICE_TYPE_SUCCESS, | ||
AMP_ADMIN_NOTICE_TYPE_WARNING, | ||
AmpAdminNotice, | ||
} from '../../components/amp-admin-notice'; | ||
import { Loading } from '../../components/loading'; | ||
import { isExternalUrl } from '../../common/helpers/is-external-url'; | ||
import { PluginsWithAmpIncompatibility } from './plugins-with-amp-incompatibility'; | ||
|
||
// Define Plugin Suppression link. | ||
const PLUGIN_SUPPRESSION_LINK = new URL( SETTINGS_LINK ); | ||
PLUGIN_SUPPRESSION_LINK.hash = 'plugin-suppression'; | ||
|
||
export function SiteScanNotice() { | ||
const { | ||
cancelSiteScan, | ||
fetchScannableUrls, | ||
isCancelled, | ||
isCompleted, | ||
isFailed, | ||
isInitializing, | ||
isReady, | ||
pluginsWithAmpIncompatibility, | ||
startSiteScan, | ||
} = useContext( SiteScan ); | ||
|
||
// Cancel scan on component unmount. | ||
useEffect( () => cancelSiteScan, [ cancelSiteScan ] ); | ||
|
||
// Fetch scannable URLs on mount. Start site scan right after the component is mounted and the scanner is ready. | ||
useEffect( () => { | ||
if ( isInitializing ) { | ||
fetchScannableUrls(); | ||
} else if ( isReady ) { | ||
startSiteScan(); | ||
} | ||
}, [ fetchScannableUrls, isInitializing, isReady, startSiteScan ] ); | ||
|
||
const commonNoticeProps = { | ||
className: 'amp-site-scan-notice', | ||
isDismissible: true, | ||
onDismiss: cancelSiteScan, | ||
}; | ||
|
||
if ( isFailed || isCancelled ) { | ||
return ( | ||
<AmpAdminNotice type={ AMP_ADMIN_NOTICE_TYPE_ERROR } { ...commonNoticeProps }> | ||
<p> | ||
{ __( 'AMP could not check your site for compatibility issues.', 'amp' ) } | ||
</p> | ||
</AmpAdminNotice> | ||
); | ||
} | ||
|
||
if ( isCompleted && pluginsWithAmpIncompatibility.length === 0 ) { | ||
return ( | ||
<AmpAdminNotice type={ AMP_ADMIN_NOTICE_TYPE_SUCCESS } { ...commonNoticeProps }> | ||
<p> | ||
{ __( 'No AMP compatibility issues detected.', 'amp' ) } | ||
</p> | ||
</AmpAdminNotice> | ||
); | ||
} | ||
|
||
if ( isCompleted && pluginsWithAmpIncompatibility.length > 0 ) { | ||
return ( | ||
<AmpAdminNotice type={ AMP_ADMIN_NOTICE_TYPE_WARNING } { ...commonNoticeProps }> | ||
<PluginsWithAmpIncompatibility pluginsWithAmpIncompatibility={ pluginsWithAmpIncompatibility } /> | ||
<div className="amp-site-scan-notice__cta"> | ||
<a href={ PLUGIN_SUPPRESSION_LINK } className="button"> | ||
{ __( 'Review Plugin Suppression', 'amp' ) } | ||
</a> | ||
<a | ||
href={ AMP_COMPATIBLE_PLUGINS_URL } | ||
className="button" | ||
{ ...isExternalUrl( AMP_COMPATIBLE_PLUGINS_URL ) ? { target: '_blank', rel: 'noopener noreferrer' } : {} } | ||
> | ||
{ __( 'View AMP-Compatible Plugins', 'amp' ) } | ||
</a> | ||
</div> | ||
</AmpAdminNotice> | ||
); | ||
} | ||
|
||
return ( | ||
<AmpAdminNotice type={ AMP_ADMIN_NOTICE_TYPE_INFO } { ...commonNoticeProps }> | ||
<p> | ||
{ __( 'Checking your site for AMP compatibility issues…', 'amp' ) } | ||
<Loading inline={ true } /> | ||
delawski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</p> | ||
</AmpAdminNotice> | ||
); | ||
} |
91 changes: 91 additions & 0 deletions
91
assets/src/admin/site-scan-notice/plugins-with-amp-incompatibility.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useContext } from '@wordpress/element'; | ||
import { _n, sprintf } from '@wordpress/i18n'; | ||
|
||
/** | ||
* External dependencies | ||
*/ | ||
import PropTypes from 'prop-types'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { Plugins } from '../../components/plugins-context-provider'; | ||
import { getPluginSlugFromFile } from '../../common/helpers/get-plugin-slug-from-file'; | ||
|
||
/** | ||
* Render a DETAILS element for each plugin causing AMP incompatibilities. | ||
* | ||
* @param {Object} props Component props. | ||
* @param {Array} props.pluginsWithAmpIncompatibility Array of plugins with AMP incompatibilities. | ||
*/ | ||
export function PluginsWithAmpIncompatibility( { pluginsWithAmpIncompatibility } ) { | ||
const { fetchingPlugins, plugins } = useContext( Plugins ); | ||
|
||
if ( fetchingPlugins ) { | ||
return null; | ||
} | ||
|
||
const pluginNames = Object.values( plugins ).reduce( ( accumulatedPluginNames, plugin ) => { | ||
return { | ||
...accumulatedPluginNames, | ||
[ getPluginSlugFromFile( plugin.plugin ) ]: plugin.name, | ||
}; | ||
}, {} ); | ||
|
||
return ( | ||
<> | ||
<p> | ||
{ _n( | ||
'AMP compatibility issues discovered with the following plugin:', | ||
'AMP compatibility issues discovered with the following plugins:', | ||
pluginsWithAmpIncompatibility.length, | ||
'amp', | ||
) } | ||
</p> | ||
{ pluginsWithAmpIncompatibility.map( ( pluginWithAmpIncompatibility ) => ( | ||
<details | ||
key={ pluginWithAmpIncompatibility.slug } | ||
className="amp-site-scan-notice__plugin-details" | ||
> | ||
<summary | ||
className="amp-site-scan-notice__plugin-summary" | ||
dangerouslySetInnerHTML={ { | ||
__html: sprintf( | ||
/* translators: 1: plugin name; 2: number of URLs with AMP validation issues. */ | ||
_n( | ||
'<b>%1$s</b> on %2$d URL', | ||
'<b>%1$s</b> on %2$d URLs', | ||
pluginWithAmpIncompatibility.urls.length, | ||
'amp', | ||
), | ||
pluginNames[ pluginWithAmpIncompatibility.slug ], | ||
pluginWithAmpIncompatibility.urls.length, | ||
), | ||
} } | ||
/> | ||
<ul className="amp-site-scan-notice__urls-list"> | ||
{ pluginWithAmpIncompatibility.urls.map( ( url ) => ( | ||
<li key={ url }> | ||
<a href={ url } target="_blank" rel="noopener noreferrer"> | ||
{ url } | ||
</a> | ||
Comment on lines
+72
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it'd be best to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
</li> | ||
) ) } | ||
</ul> | ||
</details> | ||
) ) } | ||
</> | ||
); | ||
} | ||
|
||
PluginsWithAmpIncompatibility.propTypes = { | ||
pluginsWithAmpIncompatibility: PropTypes.arrayOf( | ||
PropTypes.shape( { | ||
slug: PropTypes.string, | ||
urls: PropTypes.arrayOf( PropTypes.string ), | ||
} ), | ||
), | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
.amp-site-scan-notice__cta { | ||
display: flex; | ||
flex-flow: row; | ||
margin: 0.5rem 0 1rem; | ||
|
||
> .button + .button { | ||
margin-left: 0.5rem; | ||
} | ||
} | ||
|
||
.amp-site-scan-notice__plugin-details { | ||
margin: 0.5rem 0; | ||
} | ||
|
||
.amp-site-scan-notice__plugin-summary { | ||
cursor: pointer; | ||
} | ||
|
||
.amp-site-scan-notice__urls-list { | ||
margin: 0.5rem 0 1rem; | ||
padding: 0 1rem; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's no need to explicitly add
noopener
ifnoreferrer
is already there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, it seems that
noopener noreferrer
is a common practice, e.g. the<ExternalLink>
from@wordpress/components
sets both rel types (plus anexternal
type).If that's the case, let's keep both
noopener
andnoreferrer
.