Treat errors caused by core/shortcode blocks like non-block issues

assets/src/block-validation/__mocks__/amp-block-validation.js

@@ -1,23 +1,23 @@
 module.exports = {
 	blockSources: {
 		'my-plugin/test-block': {
-			source: 'plugin',
+			type: 'plugin',
 			title: 'My plugin',
 		},
 		'my-mu-plugin/test-block': {
-			source: 'mu-plugin',
+			type: 'mu-plugin',
 			title: 'My MU plugin',
 		},
 		'my-theme/test-block': {
-			source: 'theme',
+			type: 'theme',
 			title: 'My theme',
 		},
 		'core/test-block': {
-			source: '',
+			type: '',
 			title: 'WordPress core',
 		},
 		'unknown/test-block': {
-			source: '',
+			type: '',
 			name: '',
 		},
 	},

assets/src/block-validation/components/error/error-content.js

@@ -24,6 +24,13 @@ import AMPDelete from '../../../../images/amp-delete.svg';
 import { getErrorSourceTitle } from './get-error-source-title';
 
 /**
+ * Display error source title.
+ *
+ * Error title not directly related to the post content are determined by the `getErrorSourceTitle` helper function.
+ * For errors that are generated by blocks in the post content (`clientId` is defined), use the `blockSources` object.
+ * The exception is `core/shortcode` block that may be mistakenly attributed to WordPress core while in fact it is
+ * generated by a plugin or theme.
+ *
  * @param {Object}   props
  * @param {string}   props.clientId      Error client ID.
  * @param {string}   props.blockTypeName Block type name.
@@ -34,8 +41,8 @@ function ErrorSource( { clientId, blockTypeName, sources } ) {
 
 	const blockSource = blockSources?.[ blockTypeName ];
 
-	if ( clientId ) {
-		switch ( blockSource?.source ) {
+	if ( clientId && 'core/shortcode' !== blockTypeName ) {
+		switch ( blockSource?.type ) {
 			case 'plugin':
 				source = sprintf(
 					/* translators: %s: plugin name. */
@@ -61,15 +68,13 @@ function ErrorSource( { clientId, blockTypeName, sources } ) {
 				break;
 
 			default:
-				source = blockSource?.title || getErrorSourceTitle( sources );
+				source = blockSource?.title;
 				break;
 		}
-	} else {
-		source = getErrorSourceTitle( sources );
 	}
 
 	if ( ! source ) {
-		source = __( 'Unknown', 'amp' );
+		source = getErrorSourceTitle( sources );
 	}
 
 	return (

assets/src/block-validation/components/error/get-error-source-title.js

@@ -9,14 +9,14 @@ import { pluginNames, themeName, themeSlug } from 'amp-block-validation';
 import { __, sprintf } from '@wordpress/i18n';
 
 /**
- * Attempts to get the title of the plugin or theme responsible for an error.
- *
- * Adapted from AMP_Validated_URL_Post_Type::render_sources_column PHP method.
+ * Retrieve sources keyed by type.
  *
  * @param {Object[]} sources Error source details from the PHP backtrace.
+ * @return {{core: *[], plugin: *[], 'mu-plugin': *[], blocks: *[], theme: *[], embed: *[]}} Keyed sources.
  */
-export function getErrorSourceTitle( sources = [] ) {
+function getKeyedSources( sources ) {
 	const keyedSources = { theme: [], plugin: [], 'mu-plugin': [], embed: [], core: [], blocks: [] };
+
 	for ( const source of sources ) {
 		if ( source.type && source.type in keyedSources ) {
 			keyedSources[ source.type ].push( source );
@@ -25,16 +25,32 @@ export function getErrorSourceTitle( sources = [] ) {
 		}
 	}
 
+	return keyedSources;
+}
+
+/**
+ * Attempts to get the title of the plugin or theme responsible for an error.
+ *
+ * Adapted from AMP_Validated_URL_Post_Type::render_sources_column PHP method.
+ *
+ * @param {Object[]} sources Error source details from the PHP backtrace.
+ */
+export function getErrorSourceTitle( sources = [] ) {
+	const keyedSources = getKeyedSources( sources );
 	const output = [];
-	const uniquePluginNames = [ ...new Set( keyedSources.plugin.map( ( { name } ) => name ) ) ];
-	const muPluginNames = [ ...new Set( keyedSources[ 'mu-plugin' ].map( ( { name } ) => name ) ) ];
-	const combinedPluginNames = [ ...uniquePluginNames, ...muPluginNames ];
+	const uniquePluginNames = new Set( keyedSources.plugin.map( ( { name } ) => name ) );
+	const muPluginNames = new Set( keyedSources[ 'mu-plugin' ].map( ( { name } ) => name ) );
+	let combinedPluginNames = [ ...uniquePluginNames, ...muPluginNames ];
+
+	if ( combinedPluginNames.length > 1 ) {
+		combinedPluginNames = combinedPluginNames.filter( ( slug ) => slug !== 'gutenberg' );
+	}
 
 	if ( 1 === combinedPluginNames.length ) {
 		output.push( pluginNames[ combinedPluginNames[ 0 ] ] || combinedPluginNames[ 0 ] );
 	} else {
-		const pluginCount = uniquePluginNames.length;
-		const muPluginCount = muPluginNames.length;
+		const pluginCount = uniquePluginNames.size;
+		const muPluginCount = muPluginNames.size;
 
 		if ( 0 < pluginCount ) {
 			output.push( sprintf( '%1$s (%2$d)', __( 'Plugins', 'amp' ), pluginCount ) );
@@ -70,5 +86,9 @@ export function getErrorSourceTitle( sources = [] ) {
 		output.push( __( 'Core', 'amp' ) );
 	}
 
+	if ( 0 === output.length && 0 < sources.length ) {
+		output.push( __( 'Unknown', 'amp' ) );
+	}
+
 	return output.join( ', ' );
 }

assets/src/block-validation/components/error/test/error.js

@@ -23,7 +23,7 @@ import { registerBlockType, createBlock } from '@wordpress/blocks';
 import { Error } from '../index';
 import { createStore } from '../../../store';
 
-let container, pluginBlock, themeBlock, coreBlock, unknownBlock;
+let container, pluginBlock, muPluginBlock, themeBlock, coreBlock, unknownBlock;
 
 const TEST_PLUGIN_BLOCK = 'my-plugin/test-block';
 const TEST_MU_PLUGIN_BLOCK = 'my-mu-plugin/test-block';
@@ -70,11 +70,12 @@ registerBlockType( TEST_UNKNOWN_BLOCK, {
 
 function createTestStoreAndBlocks() {
 	pluginBlock = createBlock( TEST_PLUGIN_BLOCK, {} );
+	muPluginBlock = createBlock( TEST_MU_PLUGIN_BLOCK, {} );
 	themeBlock = createBlock( TEST_THEME_BLOCK, {} );
 	coreBlock = createBlock( TEST_CORE_BLOCK, {} );
 	unknownBlock = createBlock( TEST_UNKNOWN_BLOCK, {} );
 
-	dispatch( 'core/block-editor' ).insertBlocks( [ pluginBlock, themeBlock, coreBlock, unknownBlock ] );
+	dispatch( 'core/block-editor' ).insertBlocks( [ pluginBlock, muPluginBlock, themeBlock, coreBlock, unknownBlock ] );
 
 	createStore( {
 		reviewLink: 'http://site.test/wp-admin',
@@ -90,6 +91,17 @@ function createTestStoreAndBlocks() {
 					sources: [],
 				},
 			},
+			{
+				clientId: muPluginBlock.clientId,
+				code: 'DISALLOWED_TAG',
+				status: 3,
+				term_id: 12,
+				title: 'Invalid script: <code>jquery.js</code>',
+				error: {
+					type: 'js_error',
+					sources: [],
+				},
+			},
 			{
 				clientId: themeBlock.clientId,
 				code: 'DISALLOWED_TAG',
@@ -133,6 +145,9 @@ function getTestBlock( type ) {
 		case 'removed':
 			return pluginBlock;
 
+		case 'mu-plugin':
+			return muPluginBlock;
+
 		case 'theme':
 			return themeBlock;
 

assets/src/block-validation/components/error/test/get-error-source-title.js

@@ -45,6 +45,30 @@ describe( 'getErrorSorceTitle', () => {
 		] ) ).toBe( 'Plugins (2), Must-use plugins (2)' );
 	} );
 
+	it( 'does not return Gutenberg if it is not the only plugin', () => {
+		expect( getErrorSourceTitle( [
+			{
+				type: 'plugin',
+				name: 'gutenberg',
+			},
+			{
+				type: 'plugin',
+				name: 'test-plugin',
+			},
+		] ) ).toBe( 'Test plugin' );
+
+		expect( getErrorSourceTitle( [
+			{
+				type: 'plugin',
+				name: 'gutenberg',
+			},
+			{
+				type: 'mu-plugin',
+				name: 'test-mu-plugin',
+			},
+		] ) ).toBe( 'Test MU plugin' );
+	} );
+
 	it( 'returns theme name if theme is source', () => {
 		expect( getErrorSourceTitle(
 			[
@@ -67,6 +91,16 @@ describe( 'getErrorSorceTitle', () => {
 		) ).toBe( 'Inactive theme(s)' );
 	} );
 
+	it( 'returns block name for block', () => {
+		expect( getErrorSourceTitle(
+			[
+				{
+					block_name: 'Some Block',
+				},
+			],
+		) ).toBe( 'Some Block' );
+	} );
+
 	it( 'returns Embed for embed', () => {
 		expect( getErrorSourceTitle( [
 			{
@@ -88,4 +122,13 @@ describe( 'getErrorSorceTitle', () => {
 			},
 		] ) ).toBe( 'Core' );
 	} );
+
+	it( 'returns Unknown for unknown sources', () => {
+		expect( getErrorSourceTitle( [
+			{
+				type: 'unknown-type',
+				name: 'core source',
+			},
+		] ) ).toBe( 'Unknown' );
+	} );
 } );

tests/e2e/specs/admin/amp-options.js

@@ -7,7 +7,7 @@ import { visitAdminPage, activateTheme, installTheme } from '@wordpress/e2e-test
  * Internal dependencies
  */
 import { completeWizard, cleanUpSettings, clickMode, scrollToElement } from '../../utils/onboarding-wizard-utils';
-import { setTemplateMode } from '../../utils/amp-settings-utils';
+import { cleanUpValidatedUrls, setTemplateMode } from '../../utils/amp-settings-utils';
 
 describe( 'AMP settings screen newly activated', () => {
 	beforeEach( async () => {
@@ -64,10 +64,13 @@ describe( 'Settings screen when reader theme is active theme', () => {
 } );
 
 describe( 'Mode info notices', () => {
-	const timeout = 10000;
+	beforeAll( async () => {
+		await cleanUpSettings();
+		await cleanUpValidatedUrls();
+		await completeWizard( { technical: true, mode: 'transitional' } );
+	} );
 
 	beforeEach( async () => {
-		await cleanUpSettings();
 		await visitAdminPage( 'admin.php', 'page=amp-options' );
 	} );
 
@@ -76,34 +79,24 @@ describe( 'Mode info notices', () => {
 	} );
 
 	it( 'show information in the Template Mode section if site scan results are stale', async () => {
-		// Trigger a site scan.
-		await page.waitForSelector( '#site-scan' );
-
-		const isPanelCollapsed = await page.$eval( '#site-scan .components-panel__body-toggle', ( el ) => el.ariaExpanded === 'false' );
-		if ( isPanelCollapsed ) {
-			await scrollToElement( { selector: '#site-scan .components-panel__body-toggle', click: true } );
-		}
-
-		await scrollToElement( { selector: '#site-scan .settings-site-scan__footer .is-primary', click: true } );
-		await expect( page ).toMatchElement( '#site-scan .settings-site-scan__footer .is-primary', { text: 'Rescan Site', timeout } );
-
-		await scrollToElement( { selector: '#template-modes' } );
-
-		// Confirm there is no notice about stale results.
-		const noticeXpath = '//*[@id="template-modes"]/*[contains(@class, "amp-notice--info")]/*[contains(text(), "Site Scan results are stale")]';
-
-		const noticeBefore = await page.$x( noticeXpath );
-		expect( noticeBefore ).toHaveLength( 0 );
-
-		// Change template mode to make the scan results stale.
-		await setTemplateMode( 'transitional' );
-
-		await page.waitForSelector( '.settings-site-scan__footer .is-primary', { timeout } );
-
-		await scrollToElement( { selector: '#template-modes' } );
-
-		const noticeAfter = await page.$x( noticeXpath );
-		expect( noticeAfter ).toHaveLength( 1 );
+		await expect( page ).toMatchElement( 'h2', { text: 'Template Mode' } );
+
+		// When there are no site scan results, no notice in the Template Mode section should be displayed.
+		await expect( page ).toMatchElement( 'button', { text: 'Scan Site' } );
+		await expect( page ).not.toMatchElement( '#template-modes h2 + .amp-notice--info' );
+
+		// Scan the site.
+		await Promise.all( [
+			scrollToElement( { selector: '.settings-site-scan__footer .is-primary', click: true } ),
+			page.waitForSelector( '.settings-site-scan__status' ),
+		] );
+		await page.waitForSelector( '.settings-site-scan__footer .is-primary', { timeout: 30000 } );
+		await expect( page ).not.toMatchElement( '#template-modes h2 + .amp-notice--info' );
+
+		// Change the template mode to make the scan results stale and confirm the notice is displayed.
+		await setTemplateMode( 'standard' );
+		await page.waitForSelector( '.settings-site-scan__footer .is-primary', { timeout: 10000 } );
+		await expect( page ).toMatchElement( '#template-modes h2 + .amp-notice--info' );
 	} );
 
 	it.todo( 'shows expected notices for theme with built-in support' );

tests/e2e/specs/amp-onboarding/done.js

@@ -1,7 +1,12 @@
 /**
  * Internal dependencies
  */
-import { testCloseButton, cleanUpSettings, moveToDoneScreen } from '../../utils/onboarding-wizard-utils';
+import {
+	testCloseButton,
+	cleanUpSettings,
+	moveToDoneScreen,
+	scrollToElement,
+} from '../../utils/onboarding-wizard-utils';
 
 async function testCommonDoneStepElements() {
 	await expect( page ).toMatchElement( 'h1', { text: 'Done' } );
@@ -21,7 +26,10 @@ async function testCommonDoneStepElements() {
 
 	const originalIframeSrc = await page.$eval( '.done__preview-iframe', ( e ) => e.getAttribute( 'src' ) );
 
-	await expect( page ).toClick( '.done__links-container a:not([class*="--active"])' );
+	await Promise.all( [
+		scrollToElement( { selector: '.done__links-container a:not([class*="--active"])', click: true } ),
+		page.waitForXPath( `//iframe[@class="done__preview-iframe"][not(@src="${ originalIframeSrc }")]` ),
+	] );
 
 	const updatedIframeSrc = await page.$eval( '.done__preview-iframe', ( e ) => e.getAttribute( 'src' ) );
 
@@ -54,12 +62,13 @@ describe( 'Done', () => {
 		await expect( page ).toMatchElement( 'p', { text: /Transitional mode/i } );
 		await expect( page ).toMatchElement( '.done__preview-container input[type="checkbox"]:checked' );
 
-		await page.waitForSelector( '.done__preview-iframe' );
 		const originalIframeSrc = await page.$eval( '.done__preview-iframe', ( e ) => e.getAttribute( 'src' ) );
 
-		await expect( page ).toClick( '.done__preview-container input[type="checkbox"]' );
+		await Promise.all( [
+			scrollToElement( { selector: '.done__preview-container input[type="checkbox"]', click: true } ),
+			page.waitForXPath( `//iframe[@class="done__preview-iframe"][not(@src="${ originalIframeSrc }")]` ),
+		] );
 
-		await page.waitForSelector( '.done__preview-iframe' );
 		const updatedIframeSrc = await page.$eval( '.done__preview-iframe', ( e ) => e.getAttribute( 'src' ) );
 
 		expect( updatedIframeSrc ).not.toBe( originalIframeSrc );

tests/e2e/utils/amp-settings-utils.js

@@ -70,3 +70,24 @@ export async function uninstallPlugin( slug ) {
 		await _uninstallPlugin( slug );
 	}
 }
+
+export async function cleanUpValidatedUrls() {
+	await switchUserToAdmin();
+	await visitAdminPage( 'edit.php', 'post_type=amp_validated_url' );
+	await page.waitForSelector( 'h1' );
+
+	const bulkSelector = await page.$( '#bulk-action-selector-top' );
+
+	if ( ! bulkSelector ) {
+		return;
+	}
+
+	await page.waitForSelector( '[id^=cb-select-all-]' );
+	await page.click( '[id^=cb-select-all-]' );
+
+	await page.select( '#bulk-action-selector-top', 'delete' );
+
+	await page.click( '#doaction' );
+	await page.waitForXPath( '//*[contains(@class, "updated notice")]/p[contains(text(), "forgotten")]' );
+	await switchUserToTest();
+}