From f14005a12311530afce095c7c61aee05ea6d489e Mon Sep 17 00:00:00 2001
From: Steven Dufresne <steve.dufresne@automattic.com>
Date: Sat, 4 Jul 2020 03:51:43 +0900
Subject: [PATCH] Block Directory: Throw error if we have an issue registering
 blocks. (#23439)

* Throw error if we have an issue registering blocks.

* Remove empty newline.

* Try out more error handling.

* Add typerror.

* Slight refactor.

* Add `isInstallable` property to disable the Add button on blocks with fatal errors

* Update the copy of error messages

* Remove variable abbreviation.

* Move the state shape from action to reducer.

* Fix broken tests.

* Fix typo in test.

* Fix margin bottom for content.

Co-authored-by: Kelly Dwan <ryelle@users.noreply.github.com>
---
 .../downloadable-block-header/index.js        |  7 +--
 .../downloadable-block-header/test/index.js   | 20 ++++++-
 .../downloadable-block-list-item/index.js     | 15 +++++-
 .../test/__snapshots__/index.js.snap          |  2 +
 .../test/index.js                             | 10 ++++
 .../downloadable-block-notice/index.js        |  9 +++-
 .../downloadable-block-notice/test/index.js   |  4 +-
 packages/block-directory/src/store/actions.js | 43 ++++++++++++---
 packages/block-directory/src/store/reducer.js | 13 ++++-
 .../block-directory/src/store/selectors.js    |  2 +-
 .../block-directory/src/store/test/actions.js |  9 ++--
 .../block-directory/src/store/test/reducer.js | 53 ++++++++++++-------
 .../src/store/test/selectors.js               | 11 ++--
 13 files changed, 151 insertions(+), 47 deletions(-)

diff --git a/packages/block-directory/src/components/downloadable-block-header/index.js b/packages/block-directory/src/components/downloadable-block-header/index.js
index 1bd1f002229aed..9508ee940f86ba 100644
--- a/packages/block-directory/src/components/downloadable-block-header/index.js
+++ b/packages/block-directory/src/components/downloadable-block-header/index.js
@@ -15,7 +15,8 @@ function DownloadableBlockHeader( {
 	title,
 	rating,
 	ratingCount,
-	isLoading,
+	isLoading = false,
+	isInstallable = true,
 	onClick,
 } ) {
 	return (
@@ -31,10 +32,10 @@ function DownloadableBlockHeader( {
 			<Button
 				isSecondary
 				isBusy={ isLoading }
-				disabled={ isLoading }
+				disabled={ isLoading || ! isInstallable }
 				onClick={ ( event ) => {
 					event.preventDefault();
-					if ( ! isLoading ) {
+					if ( ! isLoading && isInstallable ) {
 						onClick();
 					}
 				} }
diff --git a/packages/block-directory/src/components/downloadable-block-header/test/index.js b/packages/block-directory/src/components/downloadable-block-header/test/index.js
index 1562c726dd2f1c..5eecbf48effe42 100644
--- a/packages/block-directory/src/components/downloadable-block-header/test/index.js
+++ b/packages/block-directory/src/components/downloadable-block-header/test/index.js
@@ -17,7 +17,8 @@ import { pluginWithIcon } from './fixtures';
 const getContainer = (
 	{ icon, title, rating, ratingCount },
 	onClick = jest.fn(),
-	isLoading = false
+	isLoading = false,
+	isInstallable = true
 ) => {
 	return shallow(
 		<DownloadableBlockHeader
@@ -27,6 +28,7 @@ const getContainer = (
 			rating={ rating }
 			ratingCount={ ratingCount }
 			isLoading={ isLoading }
+			isInstallable={ isInstallable }
 		/>
 	);
 };
@@ -54,5 +56,21 @@ describe( 'DownloadableBlockHeader', () => {
 			expect( event.preventDefault ).toHaveBeenCalled();
 			expect( onClickMock ).toHaveBeenCalledTimes( 0 );
 		} );
+
+		test( 'should not trigger the onClick function if not installable', () => {
+			const onClickMock = jest.fn();
+			const wrapper = getContainer(
+				pluginWithIcon,
+				onClickMock,
+				false,
+				false
+			);
+			const event = {
+				preventDefault: jest.fn(),
+			};
+			wrapper.find( Button ).simulate( 'click', event );
+			expect( onClickMock ).toHaveBeenCalledTimes( 0 );
+			expect( event.preventDefault ).toHaveBeenCalled();
+		} );
 	} );
 } );
diff --git a/packages/block-directory/src/components/downloadable-block-list-item/index.js b/packages/block-directory/src/components/downloadable-block-list-item/index.js
index 6473359069a100..5d35741af51384 100644
--- a/packages/block-directory/src/components/downloadable-block-list-item/index.js
+++ b/packages/block-directory/src/components/downloadable-block-list-item/index.js
@@ -12,8 +12,18 @@ import DownloadableBlockInfo from '../downloadable-block-info';
 import DownloadableBlockNotice from '../downloadable-block-notice';
 
 export default function DownloadableBlockListItem( { item, onClick } ) {
-	const isLoading = useSelect(
-		( select ) => select( 'core/block-directory' ).isInstalling( item.id ),
+	const { isLoading, isInstallable } = useSelect(
+		( select ) => {
+			const { isInstalling, getErrorNoticeForBlock } = select(
+				'core/block-directory'
+			);
+			const notice = getErrorNoticeForBlock( item.id );
+			const hasFatal = notice && notice.isFatal;
+			return {
+				isLoading: isInstalling( item.id ),
+				isInstallable: ! hasFatal,
+			};
+		},
 		[ item ]
 	);
 
@@ -41,6 +51,7 @@ export default function DownloadableBlockListItem( { item, onClick } ) {
 						rating={ rating }
 						ratingCount={ ratingCount }
 						isLoading={ isLoading }
+						isInstallable={ isInstallable }
 					/>
 				</header>
 				<section className="block-directory-downloadable-block-list-item__body">
diff --git a/packages/block-directory/src/components/downloadable-block-list-item/test/__snapshots__/index.js.snap b/packages/block-directory/src/components/downloadable-block-list-item/test/__snapshots__/index.js.snap
index cf01ac0eaede76..59ec11f93e6dab 100644
--- a/packages/block-directory/src/components/downloadable-block-list-item/test/__snapshots__/index.js.snap
+++ b/packages/block-directory/src/components/downloadable-block-list-item/test/__snapshots__/index.js.snap
@@ -12,6 +12,8 @@ exports[`DownloadableBlockListItem should render a block item 1`] = `
     >
       <DownloadableBlockHeader
         icon="block-default"
+        isInstallable={true}
+        isLoading={false}
         onClick={[MockFunction]}
         rating={5}
         title="Boxer"
diff --git a/packages/block-directory/src/components/downloadable-block-list-item/test/index.js b/packages/block-directory/src/components/downloadable-block-list-item/test/index.js
index 3e2d0024cee682..ef96da905e0830 100644
--- a/packages/block-directory/src/components/downloadable-block-list-item/test/index.js
+++ b/packages/block-directory/src/components/downloadable-block-list-item/test/index.js
@@ -3,6 +3,11 @@
  */
 import { shallow } from 'enzyme';
 
+/**
+ * WordPress dependencies
+ */
+import { useSelect } from '@wordpress/data';
+
 /**
  * Internal dependencies
  */
@@ -18,6 +23,11 @@ jest.mock( '@wordpress/data/src/components/use-select', () => {
 
 describe( 'DownloadableBlockListItem', () => {
 	it( 'should render a block item', () => {
+		useSelect.mockImplementation( () => ( {
+			isLoading: false,
+			isInstallable: true,
+		} ) );
+
 		const wrapper = shallow(
 			<DownloadableBlockListItem onClick={ jest.fn() } item={ item } />
 		);
diff --git a/packages/block-directory/src/components/downloadable-block-notice/index.js b/packages/block-directory/src/components/downloadable-block-notice/index.js
index 61f04196ed79a4..75e7d5a513bca9 100644
--- a/packages/block-directory/src/components/downloadable-block-notice/index.js
+++ b/packages/block-directory/src/components/downloadable-block-notice/index.js
@@ -23,16 +23,21 @@ export const DownloadableBlockNotice = ( { block, onClick } ) => {
 			className="block-directory-downloadable-block-notice"
 		>
 			<div className="block-directory-downloadable-block-notice__content">
-				{ errorNotice }
+				{ errorNotice.message }
 			</div>
 			<Button
 				isSmall
 				isPrimary
 				onClick={ () => {
+					if ( errorNotice.isFatal ) {
+						window.location.reload();
+						return false;
+					}
+
 					onClick( block );
 				} }
 			>
-				{ __( 'Retry' ) }
+				{ errorNotice.isFatal ? __( 'Reload' ) : __( 'Retry' ) }
 			</Button>
 		</Notice>
 	);
diff --git a/packages/block-directory/src/components/downloadable-block-notice/test/index.js b/packages/block-directory/src/components/downloadable-block-notice/test/index.js
index 55d53cd10d9181..32b3bb8c6023a9 100644
--- a/packages/block-directory/src/components/downloadable-block-notice/test/index.js
+++ b/packages/block-directory/src/components/downloadable-block-notice/test/index.js
@@ -35,7 +35,9 @@ describe( 'DownloadableBlockNotice', () => {
 		} );
 
 		it( 'should return something when there are error notices', () => {
-			useSelect.mockImplementation( () => 'Plugin not found.' );
+			useSelect.mockImplementation( () => {
+				return { message: 'Plugin not found.', isFatal: false };
+			} );
 			const wrapper = shallow(
 				<DownloadableBlockNotice
 					block={ plugin }
diff --git a/packages/block-directory/src/store/actions.js b/packages/block-directory/src/store/actions.js
index bb98dc483d3098..09e7b6b5d52b9a 100644
--- a/packages/block-directory/src/store/actions.js
+++ b/packages/block-directory/src/store/actions.js
@@ -67,12 +67,38 @@ export function* installBlockType( block ) {
 
 		yield loadAssets( assets );
 		const registeredBlocks = yield select( 'core/blocks', 'getBlockTypes' );
-		if ( ! registeredBlocks.length ) {
-			throw new Error( __( 'Unable to get block types.' ) );
+		if (
+			! registeredBlocks.length ||
+			! registeredBlocks.filter( ( i ) => i.name === block.name ).length
+		) {
+			throw new Error(
+				__( 'Error registering block. Try reloading the page.' )
+			);
 		}
+
 		success = true;
 	} catch ( error ) {
-		yield setErrorNotice( id, error.message || __( 'An error occurred.' ) );
+		let message = error.message || __( 'An error occurred.' );
+
+		// Errors we throw are fatal
+		let isFatal = error instanceof Error;
+
+		// Specific API errors that are fatal
+		const fatalAPIErrors = {
+			folder_exists: __(
+				'This block is already installed. Try reloading the page.'
+			),
+			unable_to_connect_to_filesystem: __(
+				'Error installing block. You can reload the page and try again.'
+			),
+		};
+
+		if ( fatalAPIErrors[ error.code ] ) {
+			isFatal = true;
+			message = fatalAPIErrors[ error.code ];
+		}
+
+		yield setErrorNotice( id, message, isFatal );
 	}
 	yield setIsInstalling( block.id, false );
 	return success;
@@ -154,15 +180,17 @@ export function setIsInstalling( blockId, isInstalling ) {
  * Sets an error notice string to be displayed to the user
  *
  * @param {string} blockId The ID of the block plugin. eg: my-block
- * @param {string} notice  The message shown in the notice.
+ * @param {string} message  The message shown in the notice.
+ * @param {boolean} isFatal Whether the user can recover from the error
  *
  * @return {Object} Action object.
  */
-export function setErrorNotice( blockId, notice ) {
+export function setErrorNotice( blockId, message, isFatal = false ) {
 	return {
 		type: 'SET_ERROR_NOTICE',
 		blockId,
-		notice,
+		message,
+		isFatal,
 	};
 }
 
@@ -175,8 +203,7 @@ export function setErrorNotice( blockId, notice ) {
  */
 export function clearErrorNotice( blockId ) {
 	return {
-		type: 'SET_ERROR_NOTICE',
+		type: 'CLEAR_ERROR_NOTICE',
 		blockId,
-		notice: false,
 	};
 }
diff --git a/packages/block-directory/src/store/reducer.js b/packages/block-directory/src/store/reducer.js
index 37b75b59d1e965..8d360016180976 100644
--- a/packages/block-directory/src/store/reducer.js
+++ b/packages/block-directory/src/store/reducer.js
@@ -1,3 +1,9 @@
+/**
+ * External dependencies
+ */
+
+import { omit } from 'lodash';
+
 /**
  * WordPress dependencies
  */
@@ -88,8 +94,13 @@ export const errorNotices = ( state = {}, action ) => {
 		case 'SET_ERROR_NOTICE':
 			return {
 				...state,
-				[ action.blockId ]: action.notice,
+				[ action.blockId ]: {
+					message: action.message,
+					isFatal: action.isFatal,
+				},
 			};
+		case 'CLEAR_ERROR_NOTICE':
+			return omit( state, action.blockId );
 	}
 	return state;
 };
diff --git a/packages/block-directory/src/store/selectors.js b/packages/block-directory/src/store/selectors.js
index 02dac1432d09c2..abdbb6ba5da199 100644
--- a/packages/block-directory/src/store/selectors.js
+++ b/packages/block-directory/src/store/selectors.js
@@ -136,5 +136,5 @@ export function getErrorNotices( state ) {
  * @return {string|boolean} The error text, or false if no error.
  */
 export function getErrorNoticeForBlock( state, blockId ) {
-	return state.errorNotices[ blockId ] || false;
+	return state.errorNotices[ blockId ];
 }
diff --git a/packages/block-directory/src/store/test/actions.js b/packages/block-directory/src/store/test/actions.js
index 09069c2e4d4222..71244383d624d1 100644
--- a/packages/block-directory/src/store/test/actions.js
+++ b/packages/block-directory/src/store/test/actions.js
@@ -29,9 +29,8 @@ describe( 'actions', () => {
 			const generator = installBlockType( item );
 
 			expect( generator.next().value ).toEqual( {
-				type: 'SET_ERROR_NOTICE',
+				type: 'CLEAR_ERROR_NOTICE',
 				blockId: item.id,
-				notice: false,
 			} );
 
 			expect( generator.next().value ).toEqual( {
@@ -82,9 +81,8 @@ describe( 'actions', () => {
 			const generator = installBlockType( { ...item, assets: [] } );
 
 			expect( generator.next().value ).toEqual( {
-				type: 'SET_ERROR_NOTICE',
+				type: 'CLEAR_ERROR_NOTICE',
 				blockId: item.id,
-				notice: false,
 			} );
 
 			expect( generator.next().value ).toMatchObject( {
@@ -108,9 +106,8 @@ describe( 'actions', () => {
 			const generator = installBlockType( item );
 
 			expect( generator.next().value ).toEqual( {
-				type: 'SET_ERROR_NOTICE',
+				type: 'CLEAR_ERROR_NOTICE',
 				blockId: item.id,
-				notice: false,
 			} );
 
 			expect( generator.next().value ).toEqual( {
diff --git a/packages/block-directory/src/store/test/reducer.js b/packages/block-directory/src/store/test/reducer.js
index f18335916b2335..5d563434d7fc71 100644
--- a/packages/block-directory/src/store/test/reducer.js
+++ b/packages/block-directory/src/store/test/reducer.js
@@ -87,62 +87,77 @@ describe( 'state', () => {
 	describe( 'errorNotices()', () => {
 		it( 'should set an error notice', () => {
 			const initialState = deepFreeze( {} );
+			const error = {
+				message: 'error',
+				isFatal: false,
+			};
+
 			const state = errorNotices( initialState, {
 				type: 'SET_ERROR_NOTICE',
 				blockId: 'block/has-error',
-				notice: 'Error',
+				...error,
 			} );
 
 			expect( state ).toEqual( {
-				'block/has-error': 'Error',
+				'block/has-error': error,
 			} );
 		} );
 
 		it( 'should set a second error notice', () => {
 			const initialState = deepFreeze( {
-				'block/has-error': 'Error',
+				'block/has-error': {
+					message: 'error',
+					isFatal: false,
+				},
 			} );
 			const state = errorNotices( initialState, {
 				type: 'SET_ERROR_NOTICE',
 				blockId: 'block/another-error',
-				notice: 'Error',
+				message: 'error',
+				isFatal: true,
 			} );
 
 			expect( state ).toEqual( {
-				'block/has-error': 'Error',
-				'block/another-error': 'Error',
+				'block/has-error': {
+					message: 'error',
+					isFatal: false,
+				},
+				'block/another-error': {
+					message: 'error',
+					isFatal: true,
+				},
 			} );
 		} );
 
 		it( 'should clear an existing error notice', () => {
 			const initialState = deepFreeze( {
-				'block/has-error': 'Error',
+				'block/has-error': {
+					message: 'existing error',
+					isFatal: false,
+				},
 			} );
+
 			const state = errorNotices( initialState, {
-				type: 'SET_ERROR_NOTICE',
+				type: 'CLEAR_ERROR_NOTICE',
 				blockId: 'block/has-error',
-				notice: false,
 			} );
 
-			expect( state ).toEqual( {
-				'block/has-error': false,
-			} );
+			expect( state ).toEqual( {} );
 		} );
 
 		it( 'should clear a nonexistent error notice', () => {
 			const initialState = deepFreeze( {
-				'block/has-error': 'Error',
+				'block/has-error': {
+					message: 'new error',
+					isFatal: true,
+				},
 			} );
 			const state = errorNotices( initialState, {
-				type: 'SET_ERROR_NOTICE',
+				type: 'CLEAR_ERROR_NOTICE',
 				blockId: 'block/no-error',
-				notice: false,
 			} );
 
-			expect( state ).toEqual( {
-				'block/has-error': 'Error',
-				'block/no-error': false,
-			} );
+			expect( state ).toEqual( initialState );
 		} );
 	} );
 } );
diff --git a/packages/block-directory/src/store/test/selectors.js b/packages/block-directory/src/store/test/selectors.js
index 14d72e7c7e178b..f4efdd3b507924 100644
--- a/packages/block-directory/src/store/test/selectors.js
+++ b/packages/block-directory/src/store/test/selectors.js
@@ -175,7 +175,10 @@ describe( 'selectors', () => {
 	describe( 'getErrorNoticeForBlock', () => {
 		const state = {
 			errorNotices: {
-				'block/has-error': 'Error notice',
+				'block/has-error': {
+					message: 'Error notice',
+					isFatal: false,
+				},
 			},
 		};
 
@@ -184,7 +187,9 @@ describe( 'selectors', () => {
 				state,
 				'block/has-error'
 			);
-			expect( errorNotice ).toEqual( 'Error notice' );
+			expect( errorNotice ).toEqual(
+				state.errorNotices[ 'block/has-error' ]
+			);
 		} );
 
 		it( "should retrieve no error notice for a block that doesn't have one", () => {
@@ -192,7 +197,7 @@ describe( 'selectors', () => {
 				state,
 				'block/no-error'
 			);
-			expect( errorNotice ).toEqual( false );
+			expect( errorNotice ).toEqual( undefined );
 		} );
 	} );