From 3aa606d575d003176f09d19be3fbd3e682dbab3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 7 Dec 2018 15:28:26 +0100 Subject: [PATCH 1/8] Tests: Adjusted tests to changes in upload plugin. --- tests/imageupload/imageuploadediting.js | 162 ++++++++++++----------- tests/imageupload/imageuploadprogress.js | 78 ++++++----- tests/manual/imageupload.js | 88 ++++++------ 3 files changed, 180 insertions(+), 148 deletions(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index afaafe35..0596f674 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -331,18 +331,19 @@ describe( 'ImageUploadEditing', () => { editor.execute( 'imageUpload', { file } ); model.once( '_change', () => { - expect( getViewData( view ) ).to.equal( - '[
' + - `` + - '
]' + - '

foo bar

' ); - expect( loader.status ).to.equal( 'uploading' ); - - done(); + tryExpect( done, () => { + expect( getViewData( view ) ).to.equal( + '[
' + + `` + + '
]' + + '

foo bar

' ); + expect( loader.status ).to.equal( 'uploading' ); + } ); } ); expect( loader.status ).to.equal( 'reading' ); - nativeReaderMock.mockSuccess( base64Sample ); + + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should replace read data with server response once it is present', done => { @@ -352,18 +353,18 @@ describe( 'ImageUploadEditing', () => { model.document.once( 'change', () => { model.document.once( 'change', () => { - expect( getViewData( view ) ).to.equal( - '[
]

foo bar

' - ); - expect( loader.status ).to.equal( 'idle' ); - - done(); + tryExpect( done, () => { + expect( getViewData( view ) ).to.equal( + '[
]

foo bar

' + ); + expect( loader.status ).to.equal( 'idle' ); + } ); }, { priority: 'lowest' } ); - adapterMocks[ 0 ].mockSuccess( { default: 'image.png' } ); + loader.file.then( () => adapterMocks[ 0 ].mockSuccess( { default: 'image.png' } ) ); } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should fire notification event in case of error', done => { @@ -371,17 +372,17 @@ describe( 'ImageUploadEditing', () => { const file = createNativeFileMock(); notification.on( 'show:warning', ( evt, data ) => { - expect( data.message ).to.equal( 'Reading error.' ); - expect( data.title ).to.equal( 'Upload failed' ); - evt.stop(); - - done(); + tryExpect( done, () => { + expect( data.message ).to.equal( 'Reading error.' ); + expect( data.title ).to.equal( 'Upload failed' ); + evt.stop(); + } ); }, { priority: 'high' } ); setModelData( model, '{}foo bar' ); editor.execute( 'imageUpload', { file } ); - nativeReaderMock.mockError( 'Reading error.' ); + loader.file.then( () => nativeReaderMock.mockError( 'Reading error.' ) ); } ); it( 'should not fire notification on abort', done => { @@ -396,12 +397,15 @@ describe( 'ImageUploadEditing', () => { setModelData( model, '{}foo bar' ); editor.execute( 'imageUpload', { file } ); - nativeReaderMock.abort(); - setTimeout( () => { - sinon.assert.notCalled( spy ); - done(); - }, 0 ); + loader.file.then( () => { + nativeReaderMock.abort(); + + setTimeout( () => { + sinon.assert.notCalled( spy ); + done(); + }, 0 ); + } ); } ); it( 'should throw when other error happens during upload', done => { @@ -428,13 +432,15 @@ describe( 'ImageUploadEditing', () => { // Check if error can be caught. promise.catch( catchSpy ); - nativeReaderMock.mockSuccess(); + loader.file.then( () => { + nativeReaderMock.mockSuccess(); - setTimeout( () => { - sinon.assert.calledOnce( catchSpy ); - sinon.assert.calledWithExactly( catchSpy, error ); - done(); - }, 0 ); + setTimeout( () => { + sinon.assert.calledOnce( catchSpy ); + sinon.assert.calledWithExactly( catchSpy, error ); + done(); + }, 0 ); + } ); } ); it( 'should do nothing if image does not have uploadId', () => { @@ -460,17 +466,17 @@ describe( 'ImageUploadEditing', () => { model.document.once( 'change', () => { model.document.once( 'change', () => { - expect( getModelData( model ) ).to.equal( '[]foo bar' ); - sinon.assert.calledOnce( spy ); - - done(); + tryExpect( done, () => { + expect( getModelData( model ) ).to.equal( '[]foo bar' ); + sinon.assert.calledOnce( spy ); + } ); } ); } ); - nativeReaderMock.mockError( 'Upload error.' ); + loader.file.then( () => nativeReaderMock.mockError( 'Upload error.' ) ); } ); - it( 'should abort upload if image is removed', () => { + it( 'should abort upload if image is removed', done => { const file = createNativeFileMock(); setModelData( model, '{}foo bar' ); editor.execute( 'imageUpload', { file } ); @@ -478,15 +484,20 @@ describe( 'ImageUploadEditing', () => { const abortSpy = testUtils.sinon.spy( loader, 'abort' ); expect( loader.status ).to.equal( 'reading' ); - nativeReaderMock.mockSuccess( base64Sample ); - const image = doc.getRoot().getChild( 0 ); - model.change( writer => { - writer.remove( image ); - } ); + loader.file.then( () => { + nativeReaderMock.mockSuccess( base64Sample ); + + const image = doc.getRoot().getChild( 0 ); + model.change( writer => { + writer.remove( image ); + } ); - expect( loader.status ).to.equal( 'aborted' ); - sinon.assert.calledOnce( abortSpy ); + tryExpect( done, () => { + expect( loader.status ).to.equal( 'aborted' ); + sinon.assert.calledOnce( abortSpy ); + } ); + } ); } ); it( 'should not abort and not restart upload when image is moved', () => { @@ -557,20 +568,20 @@ describe( 'ImageUploadEditing', () => { model.document.once( 'change', () => { model.document.once( 'change', () => { - expect( getViewData( view ) ).to.equal( - '[
' + - '' + - '
]

foo bar

' - ); - expect( loader.status ).to.equal( 'idle' ); - - done(); + tryExpect( done, () => { + expect( getViewData( view ) ).to.equal( + '[
' + + '' + + '
]

foo bar

' + ); + expect( loader.status ).to.equal( 'idle' ); + } ); }, { priority: 'lowest' } ); - adapterMocks[ 0 ].mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ); + loader.file.then( () => adapterMocks[ 0 ].mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ) ); } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should prevent from browser redirecting when an image is dropped on another image', () => { @@ -754,12 +765,9 @@ describe( 'ImageUploadEditing', () => { // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. ( isEdgeEnv ? it.skip : it )( 'should get file extension from base64 string', done => { editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - try { - expect( loader.file.name.split( '.' ).pop() ).to.equal( 'png' ); - done(); - } catch ( err ) { - done( err ); - } + tryExpect( done, () => { + loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'png' ) ); + } ); }, { priority: 'low' } ); setModelData( model, '[]foo' ); @@ -785,12 +793,9 @@ describe( 'ImageUploadEditing', () => { // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. ( isEdgeEnv ? it.skip : it )( 'should use fallback file extension', done => { editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - try { - expect( loader.file.name.split( '.' ).pop() ).to.equal( 'jpeg' ); - done(); - } catch ( err ) { - done( err ); - } + tryExpect( done, () => { + loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'jpeg' ) ); + } ); }, { priority: 'low' } ); setModelData( model, '[]foo' ); @@ -815,18 +820,27 @@ describe( 'ImageUploadEditing', () => { } ); // Asserts actual and expected model data. -// Note: Since this function is run inside a promise, all errors needs to be caught -// and rethrow to be correctly processed by a testing framework. // // @param {function} done Callback function to be called when assertion is done. // @param {String} actual Actual model data. // @param {String} expected Expected model data. function expectModel( done, actual, expected ) { - try { + tryExpect( done, () => { expect( actual ).to.equal( expected ); - done(); + } ); +} + +// Runs given expect function in a try-catch. It should be used only when `expect` is called as a result of a `Promise` +// resolution as all errors may be caught by tested code and needs to be rethrow to be correctly processed by a testing framework. +// +// @param {Function} doneFn Function to run when assertion is done. +// @param {Function} expectFn Function containing all assertions. +function tryExpect( doneFn, expectFn ) { + try { + expectFn(); + doneFn(); } catch ( err ) { - done( err ); + doneFn( err ); } } diff --git a/tests/imageupload/imageuploadprogress.js b/tests/imageupload/imageuploadprogress.js index 4eb71454..d71f57dc 100644 --- a/tests/imageupload/imageuploadprogress.js +++ b/tests/imageupload/imageuploadprogress.js @@ -90,17 +90,21 @@ describe( 'ImageUploadProgress', () => { editor.execute( 'imageUpload', { file: createNativeFileMock() } ); model.document.once( 'change', () => { - expect( getViewData( view ) ).to.equal( - '[
' + + try { + expect( getViewData( view ) ).to.equal( + '[
' + `` + '
' + - '
]

foo

' - ); + '
]

foo

' + ); - done(); + done(); + } catch ( err ) { + done( err ); + } }, { priority: 'lowest' } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should work correctly when there is no "reading" status and go straight to "uploading"', () => { @@ -173,17 +177,21 @@ describe( 'ImageUploadProgress', () => { model.document.once( 'change', () => { adapterMock.mockProgress( 40, 100 ); - expect( getViewData( view ) ).to.equal( - '[
' + + try { + expect( getViewData( view ) ).to.equal( + '[
' + `` + '
' + - '
]

foo

' - ); + '
]

foo

' + ); - done(); + done(); + } catch ( err ) { + done( err ); + } }, { priority: 'lowest' } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should convert image\'s "complete" uploadStatus attribute and display temporary icon', done => { @@ -194,28 +202,32 @@ describe( 'ImageUploadProgress', () => { model.document.once( 'change', () => { model.document.once( 'change', () => { - expect( getViewData( view ) ).to.equal( - '[
' + + try { + expect( getViewData( view ) ).to.equal( + '[
' + '' + '
' + - '
]

foo

' - ); + '
]

foo

' + ); - clock.tick( 3000 ); + clock.tick( 3000 ); - expect( getViewData( view ) ).to.equal( - '[
' + + expect( getViewData( view ) ).to.equal( + '[
' + '' + - '
]

foo

' - ); + '
]

foo

' + ); - done(); + done(); + } catch ( err ) { + done( err ); + } }, { priority: 'lowest' } ); - adapterMock.mockSuccess( { default: 'image.png' } ); + loader.file.then( () => adapterMock.mockSuccess( { default: 'image.png' } ) ); } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); it( 'should allow to customize placeholder image', () => { @@ -281,18 +293,22 @@ describe( 'ImageUploadProgress', () => { model.document.once( 'change', () => { model.document.once( 'change', () => { - expect( getViewData( view ) ).to.equal( - '[
' + + try { + expect( getViewData( view ) ).to.equal( + '[
' + '' + - '
]

foo

' - ); + '
]

foo

' + ); - done(); + done(); + } catch ( err ) { + done( err ); + } }, { priority: 'lowest' } ); - adapterMock.mockSuccess( { default: 'image.png' } ); + loader.file.then( () => adapterMock.mockSuccess( { default: 'image.png' } ) ); } ); - nativeReaderMock.mockSuccess( base64Sample ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); } ); diff --git a/tests/manual/imageupload.js b/tests/manual/imageupload.js index 2ccfd07d..7f52e444 100644 --- a/tests/manual/imageupload.js +++ b/tests/manual/imageupload.js @@ -51,52 +51,54 @@ ClassicEditor } ); function createProgressButton( loader, adapterMock ) { - const fileName = loader.file.name; - const container = document.createElement( 'div' ); - const progressInfo = document.createElement( 'span' ); - progressInfo.innerHTML = `File: ${ fileName }. Progress: 0%.`; - const progressButton = document.createElement( 'button' ); - const errorButton = document.createElement( 'button' ); - const abortButton = document.createElement( 'button' ); - progressButton.innerHTML = 'Upload progress'; - errorButton.innerHTML = 'Simulate error'; - abortButton.innerHTML = 'Simulate aborting'; - - container.appendChild( progressButton ); - container.appendChild( errorButton ); - container.appendChild( abortButton ); - container.appendChild( progressInfo ); - - buttonContainer.appendChild( container ); - - let progress = 0; - const total = 500; - progressButton.addEventListener( 'click', () => { - progress += 100; - adapterMock.mockProgress( progress, total ); - - if ( progress == total ) { + loader.file.then( file => { + const fileName = file.name; + const container = document.createElement( 'div' ); + const progressInfo = document.createElement( 'span' ); + progressInfo.innerHTML = `File: ${ fileName }. Progress: 0%.`; + const progressButton = document.createElement( 'button' ); + const errorButton = document.createElement( 'button' ); + const abortButton = document.createElement( 'button' ); + progressButton.innerHTML = 'Upload progress'; + errorButton.innerHTML = 'Simulate error'; + abortButton.innerHTML = 'Simulate aborting'; + + container.appendChild( progressButton ); + container.appendChild( errorButton ); + container.appendChild( abortButton ); + container.appendChild( progressInfo ); + + buttonContainer.appendChild( container ); + + let progress = 0; + const total = 500; + progressButton.addEventListener( 'click', () => { + progress += 100; + adapterMock.mockProgress( progress, total ); + + if ( progress == total ) { + disableButtons(); + adapterMock.mockSuccess( { default: './sample.jpg' } ); + } + + progressInfo.innerHTML = `File: ${ fileName }. Progress: ${ loader.uploadedPercent }%.`; + } ); + + errorButton.addEventListener( 'click', () => { + adapterMock.mockError( 'Upload error!' ); disableButtons(); - adapterMock.mockSuccess( { default: './sample.jpg' } ); - } - - progressInfo.innerHTML = `File: ${ fileName }. Progress: ${ loader.uploadedPercent }%.`; - } ); + } ); - errorButton.addEventListener( 'click', () => { - adapterMock.mockError( 'Upload error!' ); - disableButtons(); - } ); + abortButton.addEventListener( 'click', () => { + loader.abort(); + disableButtons(); + } ); - abortButton.addEventListener( 'click', () => { - loader.abort(); - disableButtons(); + function disableButtons() { + progressButton.setAttribute( 'disabled', 'true' ); + errorButton.setAttribute( 'disabled', 'true' ); + abortButton.setAttribute( 'disabled', 'true' ); + } } ); - - function disableButtons() { - progressButton.setAttribute( 'disabled', 'true' ); - errorButton.setAttribute( 'disabled', 'true' ); - abortButton.setAttribute( 'disabled', 'true' ); - } } From d7ac45fab745ff15c2423e087e26cebcd5fff6d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 17 Dec 2018 15:07:51 +0100 Subject: [PATCH 2/8] Pass promise instead of a file instance to loader while processing pasted base64/blob images. --- src/imageupload/imageuploadediting.js | 31 ++--- src/imageupload/utils.js | 18 +-- tests/imageupload/imageuploadediting.js | 156 ++++++++++++++++++------ 3 files changed, 136 insertions(+), 69 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 52dc8c0d..d327dd6d 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -101,36 +101,25 @@ export default class ImageUploadEditing extends Plugin { this.listenTo( editor.plugins.get( 'Clipboard' ), 'inputTransformation', ( evt, data ) => { const fetchableImages = Array.from( editor.editing.view.createRangeIn( data.content ) ) .filter( value => isLocalImage( value.item ) && !value.item.getAttribute( 'uploadProcessed' ) ) - .map( value => fetchLocalImage( value.item ) ); + .map( value => { return { promise: fetchLocalImage( value.item ), imageElement: value.item }; } ); if ( !fetchableImages.length ) { return; } - evt.stop(); + const writer = new UpcastWriter(); - Promise.all( fetchableImages ).then( items => { - const writer = new UpcastWriter(); + for ( const fetchableImage of fetchableImages ) { + // Set attribute marking that the image was processed already. + writer.setAttribute( 'uploadProcessed', true, fetchableImage.imageElement ); - for ( const item of items ) { - if ( !item.file ) { - // Failed to fetch image or create a file instance, remove image element. - writer.remove( item.image ); - } else { - // Set attribute marking the image as processed. - writer.setAttribute( 'uploadProcessed', true, item.image ); + const loader = fileRepository.createLoader( fetchableImage.promise ); - const loader = fileRepository.createLoader( item.file ); - - if ( loader ) { - writer.setAttribute( 'src', '', item.image ); - writer.setAttribute( 'uploadId', loader.id, item.image ); - } - } + if ( loader ) { + writer.setAttribute( 'src', '', fetchableImage.imageElement ); + writer.setAttribute( 'uploadId', loader.id, fetchableImage.imageElement ); } - - editor.plugins.get( 'Clipboard' ).fire( 'inputTransformation', data ); - } ); + } } ); // Prevents from the browser redirecting to the dropped image. diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index fc61910c..1e460063 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -22,29 +22,29 @@ export function isImageType( file ) { } /** - * Creates a promise which fetches the image local source (base64 or blob) and returns as a `File` object. + * Creates a promise which fetches the image local source (base64 or blob) and resolves with a `File` object. * * @param {module:engine/view/element~Element} image Image which source to fetch. * @returns {Promise} A promise which resolves when image source is fetched and converted to `File` instance. - * It resolves with object holding initial image element (as `image`) and its file source (as `file`). If - * the `file` attribute is null, it means fetching failed. + * It resolves with a `File` object. If there were any errors during file processing the promise will be rejected. */ export function fetchLocalImage( image ) { - return new Promise( resolve => { + return new Promise( ( resolve, reject ) => { + const imageSrc = image.getAttribute( 'src' ); + // Fetch works asynchronously and so does not block browser UI when processing data. - fetch( image.getAttribute( 'src' ) ) + fetch( imageSrc ) .then( resource => resource.blob() ) .then( blob => { - const mimeType = getImageMimeType( blob, image.getAttribute( 'src' ) ); + const mimeType = getImageMimeType( blob, imageSrc ); const ext = mimeType.replace( 'image/', '' ); const filename = `image.${ ext }`; const file = createFileFromBlob( blob, filename, mimeType ); - resolve( { image, file } ); + file ? resolve( file ) : reject(); } ) .catch( () => { - // We always resolve a promise so `Promise.all` will not reject if one of many fetch fails. - resolve( { image, file: null } ); + reject(); } ); } ); } diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 0596f674..0af9b5c9 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -20,7 +20,7 @@ import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository'; import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; -import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; +import { getData as getViewData, stringify as stringifyView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import log from '@ckeditor/ckeditor5-utils/src/log'; import env from '@ckeditor/ckeditor5-utils/src/env'; @@ -656,11 +656,13 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should not upload and remove image if fetch failed', done => { - editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - const expected = '[]foo'; - - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + expectData( + '', + '[]foo', + '[]foo', + done, + false + ); setModelData( model, '[]foo' ); @@ -679,14 +681,22 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should upload only images which were successfully fetched and remove failed ones', done => { - editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - const expected = 'bar' + - `` + - `[]` + - 'foo'; - - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + const expectedModel = 'bar' + + '' + + '' + + '[]' + + 'foo'; + const expectedFinalModel = 'bar' + + '' + + '[]' + + 'foo'; + + expectData( + '', + expectedModel, + expectedFinalModel, + done + ); setModelData( model, '[]foo' ); @@ -717,13 +727,16 @@ describe( 'ImageUploadEditing', () => { window.File = undefined; - editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - window.File = fileFn; - - const expected = 'baz[]foo'; - - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + expectData( + '

baz

', + 'baz[]foo', + 'baz[]foo', + err => { + window.File = fileFn; + done( err ); + }, + false + ); setModelData( model, '[]foo' ); @@ -737,19 +750,15 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should not upload and remove image when `File` constructor is not supported', done => { - const fileFn = window.File; - - window.File = function() { - throw new Error( 'Function expected.' ); // Simulating Edge browser behaviour here. - }; - - editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - window.File = fileFn; - - const expected = 'baz[]foo'; - - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' ); + + expectData( + '

baz

', + 'baz[]foo', + 'baz[]foo', + done, + false + ); setModelData( model, '[]foo' ); @@ -765,8 +774,10 @@ describe( 'ImageUploadEditing', () => { // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. ( isEdgeEnv ? it.skip : it )( 'should get file extension from base64 string', done => { editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - tryExpect( done, () => { - loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'png' ) ); + loader.file.then( file => { + tryExpect( done, () => { + expect( file.name.split( '.' ).pop() ).to.equal( 'png' ); + } ); } ); }, { priority: 'low' } ); @@ -793,8 +804,10 @@ describe( 'ImageUploadEditing', () => { // Skip this test on Edge as we mock `File` object there so there is no sense in testing it. ( isEdgeEnv ? it.skip : it )( 'should use fallback file extension', done => { editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { - tryExpect( done, () => { - loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'jpeg' ) ); + loader.file.then( file => { + tryExpect( done, () => { + expect( file.name.split( '.' ).pop() ).to.equal( 'jpeg' ); + } ); } ); }, { priority: 'low' } ); @@ -817,8 +830,72 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); + + // Helper for validating clipboard and model data as a result of a paste operation. This function checks both clipboard + // data and model data synchronously (`expectedClipboardData`, `expectedModel`) and then the model data after `loader.file` + // promise is resolved (so model state after successful/failed file fetch attempt). + // + // @param {String} expectedClipboardData Expected clipboard data on `inputTransformation` event. + // @param {String} expectedModel Expected model data on `inputTransformation` event. + // @param {String} expectedModelOnFile Expected model data after all `file.loader` promises are fetched. + // @param {Function} doneFn Callback function to be called when all assertions are done or error occures. + // @param {Boolean} [onSuccess=true] If `expectedModelOnFile` data should be validated + // on `loader.file` a promise successful resolution or promise rejection. + function expectData( expectedClipboardData, expectedModel, expectedModelOnFile, doneFn, onSuccess ) { + // Check data after paste. + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', ( evt, data ) => { + const clipboardData = injectLoaderId( expectedClipboardData || '', adapterMocks ); + const modelData = injectLoaderId( expectedModel, adapterMocks ); + const finalModelData = injectLoaderId( expectedModelOnFile, adapterMocks ); + + if ( clipboardData.length ) { + expect( stringifyView( data.content ) ).to.equal( clipboardData ); + } + expect( getModelData( model ) ).to.equal( modelData ); + + if ( onSuccess !== false ) { + adapterMocks[ 0 ].loader.file.then( () => { + // Deffer so the promise could be resolved. + setTimeout( () => { + expectModel( doneFn, getModelData( model ), finalModelData ); + } ); + } ); + } else { + adapterMocks[ 0 ].loader.file.then( () => { + expect.fail( 'The `loader.file` should be rejected.' ); + } ).catch( () => { + // Deffer so the promise could be resolved. + setTimeout( () => { + expectModel( doneFn, getModelData( model ), finalModelData ); + } ); + } ); + } + }, { priority: 'low' } ); + } } ); +// Replaces '#loaderX_id' parameter in the given string with a loader id. It is used +// so data string could be created before loader is initialized. +// +// @param {String} data String which have 'loader params' replaced. +// @param {Array.} adapters Adapters list. Each adapter holds a reference to a loader which id is used. +// @returns {String} Data string with 'loader params' replaced. +function injectLoaderId( data, adapters ) { + let newData = data; + + if ( newData.includes( '#loader1_id' ) ) { + newData = newData.replace( '#loader1_id', adapters[ 0 ].loader.id ); + } + if ( newData.includes( '#loader2_id' ) ) { + newData = newData.replace( '#loader2_id', adapters[ 1 ].loader.id ); + } + if ( newData.includes( '#loader3_id' ) ) { + newData = newData.replace( '#loader3_id', adapters[ 2 ].loader.id ); + } + + return newData; +} + // Asserts actual and expected model data. // // @param {function} done Callback function to be called when assertion is done. @@ -831,7 +908,8 @@ function expectModel( done, actual, expected ) { } // Runs given expect function in a try-catch. It should be used only when `expect` is called as a result of a `Promise` -// resolution as all errors may be caught by tested code and needs to be rethrow to be correctly processed by a testing framework. +// resolution. In such cases all errors may be caught by tested code and needs to be rethrow to be correctly processed +// by a testing framework. // // @param {Function} doneFn Function to run when assertion is done. // @param {Function} expectFn Function containing all assertions. From 168c5f1a7ce3dd092f0824cc5244bf1b2fe68c00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 11 Jan 2019 17:34:57 +0100 Subject: [PATCH 3/8] Tests: Prevent alert logs. --- tests/imageupload/imageuploadediting.js | 30 ++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 3caa5060..87162ec6 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -483,7 +483,7 @@ describe( 'ImageUploadEditing', () => { loader.file.then( () => nativeReaderMock.mockError( 'Upload error.' ) ); } ); - it( 'should abort upload if image is removed', done => { + it( 'should abort upload if image is removed', () => { const file = createNativeFileMock(); setModelData( model, '{}foo bar' ); editor.execute( 'imageUpload', { file } ); @@ -661,6 +661,13 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should not upload and remove image if fetch failed', done => { + const notification = editor.plugins.get( Notification ); + + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); + expectData( '', '[]foo', @@ -686,6 +693,13 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should upload only images which were successfully fetched and remove failed ones', done => { + const notification = editor.plugins.get( Notification ); + + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); + const expectedModel = 'bar' + '' + '' + @@ -732,6 +746,13 @@ describe( 'ImageUploadEditing', () => { window.File = undefined; + const notification = editor.plugins.get( Notification ); + + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); + expectData( '

baz

', 'baz[]foo', @@ -757,6 +778,13 @@ describe( 'ImageUploadEditing', () => { it( 'should not upload and remove image when `File` constructor is not supported', done => { testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' ); + const notification = editor.plugins.get( Notification ); + + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); + expectData( '

baz

', 'baz[]foo', From a964f168a1d6cec43540887022ce6c2a2cd25d2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 11 Jan 2019 17:58:50 +0100 Subject: [PATCH 4/8] Tests: Fix test for Edge. --- tests/imageupload/imageuploadediting.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 87162ec6..51c030b1 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -776,7 +776,12 @@ describe( 'ImageUploadEditing', () => { } ); it( 'should not upload and remove image when `File` constructor is not supported', done => { - testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' ); + if ( isEdgeEnv ) { + // Since on Edge `File` is already stubbed, restore it so that excpetion will be thrown. + testUtils.sinon.restore(); + } else { + testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' ); + } const notification = editor.plugins.get( Notification ); From 4be289786ecedc7a3249b61954904b3d3324de49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 14 Jan 2019 12:39:32 +0100 Subject: [PATCH 5/8] Tests: Fix stubbing for Edge test. --- tests/imageupload/imageuploadediting.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 51c030b1..5d4a88ef 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -777,8 +777,10 @@ describe( 'ImageUploadEditing', () => { it( 'should not upload and remove image when `File` constructor is not supported', done => { if ( isEdgeEnv ) { - // Since on Edge `File` is already stubbed, restore it so that excpetion will be thrown. + // Since on Edge `File` is already stubbed, restore it to it native form so that exception will be thrown. testUtils.sinon.restore(); + // Since all stubs were restored, re-stub `scrollToTheSelection`. + testUtils.sinon.stub( editor.editing.view, 'scrollToTheSelection' ).callsFake( () => {} ); } else { testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' ); } From 876cc3c6d4ec6f0b6d34e33d1cb6e867bfeaa793 Mon Sep 17 00:00:00 2001 From: Maciej Date: Tue, 15 Jan 2019 16:07:39 +0100 Subject: [PATCH 6/8] Update src/imageupload/utils.js Co-Authored-By: f1ames --- src/imageupload/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index 1e460063..ac3704e6 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -25,7 +25,7 @@ export function isImageType( file ) { * Creates a promise which fetches the image local source (base64 or blob) and resolves with a `File` object. * * @param {module:engine/view/element~Element} image Image which source to fetch. - * @returns {Promise} A promise which resolves when image source is fetched and converted to `File` instance. + * @returns {Promise.} A promise which resolves when image source is fetched and converted to `File` instance. * It resolves with a `File` object. If there were any errors during file processing the promise will be rejected. */ export function fetchLocalImage( image ) { From c89e211c2d95496151b46987c3f6bf749a4d310f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 15 Jan 2019 16:10:40 +0100 Subject: [PATCH 7/8] Refactoring. --- src/imageupload/utils.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index ac3704e6..5ebb9f07 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -43,9 +43,7 @@ export function fetchLocalImage( image ) { file ? resolve( file ) : reject(); } ) - .catch( () => { - reject(); - } ); + .catch( reject ); } ); } From f5a52f3eee29cb8aa0b5e8225d526f10b7d78ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 16 Jan 2019 16:15:47 +0100 Subject: [PATCH 8/8] Do not show warning on empty error. --- src/imageupload/imageuploadediting.js | 2 +- tests/imageupload/imageuploadediting.js | 39 ++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 15d3119c..98977b11 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -218,7 +218,7 @@ export default class ImageUploadEditing extends Plugin { } // Might be 'aborted'. - if ( loader.status == 'error' ) { + if ( loader.status == 'error' && error ) { notification.showWarning( error, { title: t( 'Upload failed' ), namespace: 'upload' diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 5d4a88ef..ee745c80 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -734,7 +734,7 @@ describe( 'ImageUploadEditing', () => { if ( counter < 3 ) { return fetch( src ); } else { - return new Promise( ( res, rej ) => rej() ); + return Promise.reject(); } } ); @@ -867,6 +867,43 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); + it( 'should not show notification when file loader failed with no error', done => { + const notification = editor.plugins.get( Notification ); + + let notificationsCount = 0; + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + notificationsCount++; + evt.stop(); + }, { priority: 'high' } ); + + // Check data after paste. + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + adapterMocks[ 0 ].loader.file.then( () => { + expect.fail( 'Promise should be rejected.' ); + } ).catch( () => { + // Deffer so the promise could be resolved. + setTimeout( () => { + expect( notificationsCount ).to.equal( 0 ); + done(); + } ); + } ); + }, { priority: 'low' } ); + + setModelData( model, '[]foo' ); + + const clipboardHtml = ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); + + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); + + // Stub `fetch` in a way that it always fails. + testUtils.sinon.stub( window, 'fetch' ).callsFake( () => Promise.reject() ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); + } ); + // Helper for validating clipboard and model data as a result of a paste operation. This function checks both clipboard // data and model data synchronously (`expectedClipboardData`, `expectedModel`) and then the model data after `loader.file` // promise is resolved (so model state after successful/failed file fetch attempt).