Merge branch 't/13566'

Author: oleq

CHANGES.md

@@ -29,6 +29,7 @@ Fixed Issues:
 * [#13554](http://dev.ckeditor.com/ticket/13554): [Edge] Fixed: Paste dialog's iframe does not receive focus on show.
 * [#13574](http://dev.ckeditor.com/ticket/13574): [Edge] Fixed: Permission denied thrown while opening editor dialogs.
 * [#13440](http://dev.ckeditor.com/ticket/13440): [Edge] Fixed: Unable to paste a widget.
+* [#13566](http://dev.ckeditor.com/ticket/13566): Fixed: Suppressed notifications in [Media Embed Base](http://ckeditor.com/addon/embedbase) plugin.
 
 Other Changes:
 

plugins/embedbase/plugin.js

@@ -349,17 +349,17 @@
 			 * was canceled or the default listener could not convert oEmbed response into embeddable HTML.
 			 */
 			_handleResponse: function( request ) {
-				if ( request.task ) {
-					request.task.done();
-				}
-
 				var evtData = {
 					url: request.url,
 					html: '',
 					response: request.response
 				};
 
 				if ( this.fire( 'handleResponse', evtData ) !== false ) {
+					if ( request.task ) {
+						request.task.done();
+					}
+
 					this._setContent( request.url, evtData.html );
 					return true;
 				} else {

plugins/notificationaggregator/plugin.js

@@ -414,6 +414,14 @@
 		 * @property {Number}
 		 */
 		this._doneWeight = 0;
+
+		/**
+		 * Indicates when the task is canceled.
+		 *
+		 * @private
+		 * @property {Boolean}
+		 */
+		this._isCanceled = false;
 	}
 
 	Task.prototype = {
@@ -430,9 +438,9 @@
 		 * @param {Number} weight Number indicating how much of the total task {@link #_weight} is done.
 		 */
 		update: function( weight ) {
-			// If task is already done there is no need to update it, and we don't expect
+			// If task is already done or canceled there is no need to update it, and we don't expect
 			// progress to be reversed.
-			if ( this.isDone() ) {
+			if ( this.isDone() || this.isCanceled() ) {
 				return;
 			}
 
@@ -455,6 +463,14 @@
 		 * Cancels the task (the task will be removed from the aggregator).
 		 */
 		cancel: function() {
+			// If task is already done or canceled.
+			if ( this.isDone() || this.isCanceled() ) {
+				return;
+			}
+
+			// Mark task as canceled.
+			this._isCanceled = true;
+
 			// We'll fire cancel event it's up to aggregator to listen for this event,
 			// and remove the task.
 			this.fire( 'canceled' );
@@ -467,6 +483,15 @@
 		 */
 		isDone: function() {
 			return this._weight === this._doneWeight;
+		},
+
+		/**
+		 * Checks if the task is canceled.
+		 *
+		 * @returns {Boolean}
+		 */
+		isCanceled: function() {
+			return this._isCanceled;
 		}
 	};
 

tests/plugins/autoembed/autoembed.js

@@ -334,5 +334,46 @@ bender.test( {
 				assert.areSame( finalizeCreationSpy.called, false, 'finalize creation was not called' );
 			}, 200 );
 		} );
+	},
+
+	'check if notifications are showed after unsuccessful embedding': function() {
+		var bot = this.editorBot,
+			editor = bot.editor,
+			firstRequest = true,
+			notificationShowSpy = sinon.spy( CKEDITOR.plugins.notification.prototype, 'show' );
+
+		// JSONP callback for embed request.
+		jsonpCallback = function( urlTemplate, urlParams, callback, errorCallback ) {
+
+			// First request will return error - so two notifications should be showed.
+			// First informing about embedding process, second about embedding error.
+			if ( firstRequest ) {
+				errorCallback();
+				firstRequest = false;
+				editor.execCommand( 'paste', 'https://foo.bar/g/notification/test/2' );
+			} else {
+				resume( function() {
+
+					// Second request returns success - one notification should be showed.
+					callback( {
+						'url': decodeURIComponent( urlParams.url ),
+						'type': 'rich',
+						'version': '1.0',
+						'html': '<img src="' + decodeURIComponent( urlParams.url ) + '">'
+					} );
+
+					notificationShowSpy.restore();
+
+					// Check if notification was showed three times.
+					assert.isTrue( notificationShowSpy.calledThrice, 'Notification should be showed three times.' );
+				} );
+			}
+		};
+
+		bot.setData( '', function() {
+			editor.focus();
+			editor.execCommand( 'paste', 'https://foo.bar/g/notification/test/1' );
+			wait();
+		} );
 	}
 } );

tests/plugins/autoembed/manual/notifications.html

@@ -0,0 +1,24 @@
+<p>Non-embeddable URLs</p>
+
+<ul>
+	<li><input type="text" value="http://ckeditor.com/"></li>
+	<li><input type="text" value="http://foo/bar/"></li>
+</ul>
+
+<p>Embeddable URLs</p>
+
+<ul>
+	<li><input type="text" value="https://placekitten.com/g/200/300"></li>
+	<li><input type="text" value="https://twitter.com/reinmarpl/status/573118615274315776"></li>
+</ul>
+
+<textarea id="editor1" cols="10" rows="10"></textarea>
+
+<script>
+	embedTools.delayJsonp();
+	CKEDITOR.replace( 'editor1', {
+		extraPlugins: 'embed',
+		height: 500,
+		width: 600
+	} );
+</script>

tests/plugins/autoembed/manual/notifications.md

@@ -0,0 +1,9 @@
+@bender-tags: tc, 4.5.2, 13566
+@bender-ui: collapsed
+@bender-include: ../../embedbase/_helpers/tools.js
+@bender-ckeditor-plugins: wysiwygarea,sourcearea,htmlwriter,entities,toolbar,elementspath,undo,clipboard,autolink,autoembed,link
+
+1. Paste one of non-embeddable links.
+1. Wait until two notifications are showed. First informing about embedding process in progress, second informing about embedding failure.
+1. Paste one of embeddable links.
+1. Check if notifications are showed and embedding is finished correctly.

tests/plugins/embedbase/definition.js

@@ -458,7 +458,7 @@ bender.test( {
 						assert.areSame( 'foo', widget.data.url, 'widget\'s url has not been changed' );
 						assert.areSame( dataWithWidget, editor.getData() );
 
-						assert.isTrue( task.isDone(), 'task is done' );
+						assert.isTrue( task.isCanceled(), 'task is canceled' );
 
 						assert.isTrue( handleResponseSpy.calledOnce, '_handleResponse was called once' );
 						assert.isFalse( handleResponseSpy.returnValues[ 0 ], '_handleResponse returned false' );

tests/plugins/notificationaggregator/task.js

@@ -158,7 +158,57 @@
 
 			assert.areSame( 1, instance.fire.callCount, 'instance.fire call count' );
 			sinon.assert.calledWithExactly( instance.fire, 'canceled' );
+		},
+
+		'test task cannot be finished twice': function() {
+			var instance = new Task( 300 ),
+				doneEventSpy = sinon.spy();
+
+			instance.on( 'done', doneEventSpy );
+			instance.update( 300 );
+			instance.update( 300 );
+
+			assert.isTrue( doneEventSpy.calledOnce, 'Done event should be fired once.' );
+		},
+
+		'test task cannot be cancelled twice': function() {
+			var instance = new Task( 300 ),
+				cancelEventSpy = sinon.spy();
+
+			instance.on( 'canceled', cancelEventSpy );
+			instance.cancel();
+			instance.cancel();
+
+			assert.isTrue( cancelEventSpy.calledOnce, 'Cancel event should be fired once.' );
+		},
+
+		'test task cannot be cancelled after being finished': function() {
+			var instance = new Task( 300 ),
+				doneEventSpy = sinon.spy(),
+				cancelEventSpy = sinon.spy();
+
+			instance.on( 'canceled', cancelEventSpy );
+			instance.on( 'done', doneEventSpy );
+			instance.update( 300 );
+			instance.cancel();
+
+			assert.isTrue( doneEventSpy.calledOnce, 'Done event should be fired once.' );
+			assert.isFalse( cancelEventSpy.called, 'Cancel event should not be fired.' );
+		},
+
+		'test task cannot be finished after being cancelled': function() {
+			var instance = new Task( 300 ),
+				doneEventSpy = sinon.spy(),
+				cancelEventSpy = sinon.spy();
+
+			instance.on( 'canceled', cancelEventSpy );
+			instance.on( 'done', doneEventSpy );
+			instance.cancel();
+			instance.update( 300 );
+
+			assert.isTrue( cancelEventSpy.calledOnce, 'Cancel event should be fired once.' );
+			assert.isFalse( doneEventSpy.called, 'Done event should not be fired.' );
 		}
 	} );
 
-} )();
+} )();