Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the CC in the Upload package #5892

Closed
oleq opened this issue Dec 3, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-upload#106 or ckeditor/ckeditor5-upload#108
Closed

Improve the CC in the Upload package #5892

oleq opened this issue Dec 3, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-upload#106 or ckeditor/ckeditor5-upload#108
Assignees
Labels
package:upload type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@oleq
Copy link
Member

oleq commented Dec 3, 2019

The CC is not 100% and we should figure out what to do about it. This issue is strictly about CC but more details about some general problems in the package can be found in #5875.

A follow-up of #5848.

@oleq oleq added package:upload type:task This issue reports a chore (non-production change) and other types of "todos". labels Dec 3, 2019
@oleq oleq mentioned this issue Dec 3, 2019
3 tasks
@Reinmar Reinmar added this to the iteration 29 milestone Dec 3, 2019
@Reinmar Reinmar self-assigned this Dec 16, 2019
@Reinmar
Copy link
Member

Reinmar commented Dec 18, 2019

OK, I spent a bit of time on this code and I'm 99% sure that this state is impossible to be reached. Whenever the file reader is aborted, the promise is rejected so we end up in catch() and not in then()

In the past, you could do abort() while loading a file and that didn't correctly reject the file promise. I think that we fixed this with @jodator some time ago and that's why then() is no longer reached with the aborted status.

I'll remove this condition.

oleq added a commit to ckeditor/ckeditor5-upload that referenced this issue Dec 19, 2019
Tests: Removed a condition which is not reachable anymore. Closes ckeditor/ckeditor5#5892.
@Reinmar
Copy link
Member

Reinmar commented Dec 19, 2019

This appeared on CI after we merged ckeditor/ckeditor5-upload#106 😂. So it seems that I was wrong.

@Reinmar Reinmar reopened this Dec 19, 2019
@Reinmar
Copy link
Member

Reinmar commented Dec 19, 2019

It's this test:

	it.only( 'should abort upload if image is removed', () => {
		const file = createNativeFileMock();
		setModelData( model, '<paragraph>{}foo bar</paragraph>' );
		editor.execute( 'imageUpload', { file } );

		const abortSpy = sinon.spy( loader, 'abort' );

		expect( loader.status ).to.equal( 'reading' );

		return 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 );
		} );
	} );

@oleq
Copy link
Member Author

oleq commented Dec 20, 2019

This is odd because CI passed for the PR...

And I ran it locally and it is clean

SUMMARY:
✔ 102 tests completed

=============================== Coverage summary ===============================
Statements   : 100% ( 215/215 )
Branches     : 100% ( 64/64 )
Functions    : 100% ( 79/79 )
Lines        : 100% ( 208/208 )

@pomek pomek assigned pomek and unassigned Reinmar Jan 10, 2020
@Reinmar Reinmar modified the milestones: iteration 29, iteration 30 Feb 21, 2020
@Reinmar Reinmar modified the milestones: iteration 30, iteration 31 Mar 10, 2020
Reinmar added a commit to ckeditor/ckeditor5-upload that referenced this issue Apr 21, 2020
Fix: Restored a condition handling an edge case in upload vs abort promise chains. Closes ckeditor/ckeditor5#5892.
mlewand pushed a commit that referenced this issue May 1, 2020
Fix: Restored a condition handling an edge case in upload vs abort promise chains. Closes #5892.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:upload type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
3 participants