Fix bug where server side upload errors disappear automatically (#7386)

Unfortunately, the general approach to add upload error messages to all blocks without changing them does not seems possible, it created a regression as described in #6957. What happens is that when we upload we create a temporary blob URL for the file so that the placeholder disappears and the uploaded file appears immediately. In this case, the placeholder is unmounted. If we get an error message from the server e.g.: not something we can validate on the client, we set an error message but the placeholder is already not visible. Then a new placeholder is rendered, but its state is empty and the error message we set is not used anywhere. This makes impossible to handle the notices in the media placeholder and forces them to be managed at the block level. This commit does the following changes: Reverts the changes by #6957 in the gallery, image, and media-placeholder. Adds upload error notices to video, audio and cover image. Solves an existing bug in the video, audio, and cover image blocks where if the upload failed, we continued with a blob URL set.
WordPress · Jun 21, 2018 · 5ea93cd · 5ea93cd
1 parent 5389ada
commit 5ea93cd
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 45 deletions.
diff --git a/core-blocks/audio/edit.js b/core-blocks/audio/edit.js
@@ -2,7 +2,7 @@
  * WordPress dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { IconButton, Toolbar } from '@wordpress/components';
+import { IconButton, Toolbar, withNotices } from '@wordpress/components';
 import { Component, Fragment } from '@wordpress/element';
 import {
 	MediaPlaceholder,
@@ -15,7 +15,7 @@ import {
  */
 import './editor.scss';
 
-export default class AudioEdit extends Component {
+class AudioEdit extends Component {
 	constructor() {
 		super( ...arguments );
 		// edit component has its own src in the state so it can be edited
@@ -27,18 +27,23 @@ export default class AudioEdit extends Component {
 
 	render() {
 		const { caption, src } = this.props.attributes;
-		const { setAttributes, isSelected, className } = this.props;
+		const { setAttributes, isSelected, className, noticeOperations, noticeUI } = this.props;
 		const { editing } = this.state;
 		const switchToEditing = () => {
 			this.setState( { editing: true } );
 		};
 		const onSelectAudio = ( media ) => {
-			if ( media && media.url ) {
-				// sets the block's attribute and updates the edit component from the
-				// selected media, then switches off the editing UI
-				setAttributes( { src: media.url, id: media.id } );
-				this.setState( { src: media.url, editing: false } );
+			if ( ! media || ! media.url ) {
+				// in this case there was an error and we should continue in the editing state
+				// previous attributes should be removed because they may be temporary blob urls
+				setAttributes( { src: undefined, id: undefined } );
+				switchToEditing();
+				return;
 			}
+			// sets the block's attribute and updates the edit component from the
+			// selected media, then switches off the editing UI
+			setAttributes( { src: media.url, id: media.id } );
+			this.setState( { src: media.url, editing: false } );
 		};
 		const onSelectUrl = ( newSrc ) => {
 			// set the block's src from the edit component's state, and switch off the editing UI
@@ -62,6 +67,8 @@ export default class AudioEdit extends Component {
 					accept="audio/*"
 					type="audio"
 					value={ this.props.attributes }
+					notices={ noticeUI }
+					onError={ noticeOperations.createErrorNotice }
 				/>
 			);
 		}
@@ -96,3 +103,5 @@ export default class AudioEdit extends Component {
 		/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
 	}
 }
+
+export default withNotices( AudioEdit );
diff --git a/core-blocks/cover-image/index.js b/core-blocks/cover-image/index.js
@@ -7,7 +7,7 @@ import classnames from 'classnames';
 /**
  * WordPress dependencies
  */
-import { IconButton, PanelBody, RangeControl, ToggleControl, Toolbar } from '@wordpress/components';
+import { IconButton, PanelBody, RangeControl, ToggleControl, Toolbar, withNotices } from '@wordpress/components';
 import { Fragment } from '@wordpress/element';
 import { __ } from '@wordpress/i18n';
 import { createBlock } from '@wordpress/blocks';
@@ -99,10 +99,16 @@ export const settings = {
 		}
 	},
 
-	edit( { attributes, setAttributes, isSelected, className } ) {
+	edit: withNotices( ( { attributes, setAttributes, isSelected, className, noticeOperations, noticeUI } ) => {
 		const { url, title, align, contentAlign, id, hasParallax, dimRatio } = attributes;
 		const updateAlignment = ( nextAlign ) => setAttributes( { align: nextAlign } );
-		const onSelectImage = ( media ) => setAttributes( { url: media.url, id: media.id } );
+		const onSelectImage = ( media ) => {
+			if ( ! media || ! media.url ) {
+				setAttributes( { url: undefined, id: undefined } );
+				return;
+			}
+			setAttributes( { url: media.url, id: media.id } );
+		};
 		const toggleParallax = () => setAttributes( { hasParallax: ! hasParallax } );
 		const setDimRatio = ( ratio ) => setAttributes( { dimRatio: ratio } );
 
@@ -193,6 +199,8 @@ export const settings = {
 						onSelect={ onSelectImage }
 						accept="image/*"
 						type="image"
+						notices={ noticeUI }
+						onError={ noticeOperations.createErrorNotice }
 					/>
 				</Fragment>
 			);
@@ -219,7 +227,7 @@ export const settings = {
 				</div>
 			</Fragment>
 		);
-	},
+	} ),
 
 	save( { attributes, className } ) {
 		const { url, title, hasParallax, dimRatio, align, contentAlign } = attributes;

diff --git a/core-blocks/gallery/edit.js b/core-blocks/gallery/edit.js
@@ -211,9 +211,8 @@ class GalleryEdit extends Component {
 						onSelect={ this.onSelectImages }
 						accept="image/*"
 						type="image"
-						disableMaxUploadErrorMessages
 						multiple
-						additionalNotices={ noticeUI }
+						notices={ noticeUI }
 						onError={ noticeOperations.createErrorNotice }
 					/>
 				</Fragment>

diff --git a/core-blocks/image/edit.js b/core-blocks/image/edit.js
@@ -26,6 +26,7 @@ import {
 	TextControl,
 	TextareaControl,
 	Toolbar,
+	withNotices,
 } from '@wordpress/components';
 import { withSelect } from '@wordpress/data';
 import {
@@ -105,7 +106,7 @@ class ImageEdit extends Component {
 	}
 
 	onSelectImage( media ) {
-		if ( ! media ) {
+		if ( ! media || ! media.url ) {
 			this.props.setAttributes( {
 				url: undefined,
 				alt: undefined,
@@ -175,7 +176,7 @@ class ImageEdit extends Component {
 	}
 
 	render() {
-		const { attributes, setAttributes, isLargeViewport, isSelected, className, maxWidth, toggleSelection } = this.props;
+		const { attributes, setAttributes, isLargeViewport, isSelected, className, maxWidth, noticeOperations, noticeUI, toggleSelection } = this.props;
 		const { url, alt, caption, align, id, href, width, height } = attributes;
 
 		const controls = (
@@ -218,6 +219,8 @@ class ImageEdit extends Component {
 						} }
 						className={ className }
 						onSelect={ this.onSelectImage }
+						notices={ noticeUI }
+						onError={ noticeOperations.createErrorNotice }
 						accept="image/*"
 						type="image"
 					/>
@@ -413,4 +416,5 @@ export default compose( [
 		};
 	} ),
 	withViewportMatch( { isLargeViewport: 'medium' } ),
+	withNotices,
 ] )( ImageEdit );
diff --git a/core-blocks/video/edit.js b/core-blocks/video/edit.js
@@ -2,7 +2,7 @@
  * WordPress dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { IconButton, Toolbar } from '@wordpress/components';
+import { IconButton, Toolbar, withNotices } from '@wordpress/components';
 import { Component, Fragment } from '@wordpress/element';
 import {
 	MediaPlaceholder,
@@ -15,7 +15,7 @@ import {
  */
 import './editor.scss';
 
-export default class VideoEdit extends Component {
+class VideoEdit extends Component {
 	constructor() {
 		super( ...arguments );
 		// edit component has its own src in the state so it can be edited
@@ -27,18 +27,23 @@ export default class VideoEdit extends Component {
 
 	render() {
 		const { caption, src } = this.props.attributes;
-		const { setAttributes, isSelected, className } = this.props;
+		const { setAttributes, isSelected, className, noticeOperations, noticeUI } = this.props;
 		const { editing } = this.state;
 		const switchToEditing = () => {
 			this.setState( { editing: true } );
 		};
 		const onSelectVideo = ( media ) => {
-			if ( media && media.url ) {
-				// sets the block's attribute and updates the edit component from the
-				// selected media, then switches off the editing UI
-				setAttributes( { src: media.url, id: media.id } );
-				this.setState( { src: media.url, editing: false } );
+			if ( ! media || ! media.url ) {
+				// in this case there was an error and we should continue in the editing state
+				// previous attributes should be removed because they may be temporary blob urls
+				setAttributes( { src: undefined, id: undefined } );
+				switchToEditing();
+				return;
 			}
+			// sets the block's attribute and updates the edit component from the
+			// selected media, then switches off the editing UI
+			setAttributes( { src: media.url, id: media.id } );
+			this.setState( { src: media.url, editing: false } );
 		};
 		const onSelectUrl = ( newSrc ) => {
 			// set the block's src from the edit component's state, and switch off the editing UI
@@ -62,6 +67,8 @@ export default class VideoEdit extends Component {
 					accept="video/*"
 					type="video"
 					value={ this.props.attributes }
+					notices={ noticeUI }
+					onError={ noticeOperations.createErrorNotice }
 				/>
 			);
 		}
@@ -96,3 +103,5 @@ export default class VideoEdit extends Component {
 		/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
 	}
 }
+
+export default withNotices( VideoEdit );
diff --git a/editor/components/media-placeholder/index.js b/editor/components/media-placeholder/index.js
@@ -12,10 +12,9 @@ import {
 	FormFileUpload,
 	Placeholder,
 	DropZone,
-	withNotices,
 } from '@wordpress/components';
 import { __, sprintf } from '@wordpress/i18n';
-import { Component, Fragment } from '@wordpress/element';
+import { Component } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -64,26 +63,13 @@ class MediaPlaceholder extends Component {
 	}
 
 	onFilesUpload( files ) {
-		/**
-		 * We use a prop named `disable`, set to `false` by default, because it makes for a nicer
-		 * component prop API. eg:
-		 *
-		 * <MediaPlaceholder disableMaxUploadErrorMessages />
-		 * instead of:
-		 * <MediaPlaceholder enableMaxUploadErrorMessages={ false } />
-		 */
-		const { onSelect, type, multiple, onError = noop, disableMaxUploadErrorMessages = false, noticeOperations } = this.props;
+		const { onSelect, type, multiple, onError } = this.props;
 		const setMedia = multiple ? onSelect : ( [ media ] ) => onSelect( media );
 		editorMediaUpload( {
 			allowedType: type,
 			filesList: files,
 			onFileChange: setMedia,
-			onError: ( errorMessage ) => {
-				onError( errorMessage );
-				if ( disableMaxUploadErrorMessages === false ) {
-					noticeOperations.createErrorNotice( errorMessage );
-				}
-			},
+			onError,
 		} );
 	}
 
@@ -99,8 +85,7 @@ class MediaPlaceholder extends Component {
 			onSelectUrl,
 			onHTMLDrop = noop,
 			multiple = false,
-			additionalNotices,
-			noticeUI,
+			notices,
 		} = this.props;
 
 		return (
@@ -110,7 +95,7 @@ class MediaPlaceholder extends Component {
 				// translators: %s: media name label e.g: "an audio","an image", "a video"
 				instructions={ sprintf( __( 'Drag %s, upload a new one or select a file from your library.' ), labels.name ) }
 				className={ classnames( 'editor-media-placeholder', className ) }
-				notices={ <Fragment>{ additionalNotices }{ noticeUI }</Fragment> }
+				notices={ notices }
 			>
 				<DropZone
 					onFilesDrop={ this.onFilesUpload }
@@ -157,4 +142,4 @@ class MediaPlaceholder extends Component {
 	}
 }
 
-export default withNotices( MediaPlaceholder );
+export default MediaPlaceholder;