Opened 3 years ago

Closed 3 years ago

#13566 closed Bug (fixed)

No further notifications after embedbase error.

Reported by: Szymon Kupś Owned by: Szymon Kupś
Priority: Normal Milestone: CKEditor 4.5.2
Component: General Version: 4.5.0
Keywords: Cc:

Description

  1. Open http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed
  2. Paste non-embeddable link: http://example.com/
  3. Wait till error notification is showed and close it.
  4. Paste embeddable link: https://www.youtube.com/watch?v=GUl9_5kK9ts

Actual result
There are no notifications about second embedding process.

Expected result
Notifications about second embedding process should be displayed as usual.

Change History (12)

comment:1 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.2
Status: newconfirmed
Version: 4.5.14.5.0

comment:2 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: confirmedassigned

comment:3 Changed 3 years ago by Szymon Kupś

I think that problem lies in _handleResponse method in embedbase plugin. Request's task is marked there as done and later on in _handleError is marked as cancelled. This creates situation where notification aggregator works in a unexpected way.

One solution can be to call task's done() method only if response was handled correctly.

Second solution that comes to my mind is to change task behaviour: cancel() and done() methods should exclude themselves. This means that task that once was marked as done cannot be cancelled and other way around.

comment:4 Changed 3 years ago by Piotrek Koszuliński

I'm more for the second solution + fixing the embedbase as well if it's easy :>. The second solution will assure that problems like this does not happen at all, also when other (e.g. 3rd party) plugins use the aggregator. The first solution would just make our code correct, which is always nice to have.

comment:5 Changed 3 years ago by Szymon Kupś

Status: assignedreview

comment:6 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I pushed branch:t/13566 with some minor improvements.

The code is good. Missing things though:

  1. Manual test for the reported TC.
  2. A unit test for the reported TC.

comment:7 Changed 3 years ago by Szymon Kupś

Status: review_failedreview

Pushed: branch:t/13566
Added unit test and manual test for the reported TC.

comment:9 Changed 3 years ago by Szymon Kupś

Status: review_failedassigned

comment:10 Changed 3 years ago by Szymon Kupś

Status: assignedreview

Pushed: branch:t/13566

Test was checking if task was done after unsuccessful request. Now embedbase is cancelling tasks on request error, so test should use task.isCanceled() method.

comment:11 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:12 Changed 3 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged git:cb014cc into master.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy