From 539a6cf1b85409eac8d327ff27cf51c1e5d53711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Szcz=C4=99=C5=9Bniak?= Date: Tue, 6 Jun 2023 12:41:37 +0200 Subject: [PATCH 1/6] Fix for uploading images. --- src/app/plugins/githubuploadadapter.js | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/app/plugins/githubuploadadapter.js b/src/app/plugins/githubuploadadapter.js index 78b7315..ede01ab 100644 --- a/src/app/plugins/githubuploadadapter.js +++ b/src/app/plugins/githubuploadadapter.js @@ -106,6 +106,38 @@ export class Adapter { // The final URL of the file is already known, even before the upload. We save it here. returnUrl = response.asset.href; + /** + * This is a copy&paste code from gitHub source code `attachment-upload.ts` + * + * It marks uploaded file as completed and return correct URL. + */ + const url = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; + const token = typeof response.asset_upload_authenticity_token == 'string' ? + response.asset_upload_authenticity_token : null; + + if ( !( url && token ) ) { + return; + } + + const form = new FormData(); + form.append( 'authenticity_token', token ); + + fetch( url, { + method: 'PUT', + body: form, + credentials: 'same-origin', + headers: { + Accept: 'application/json', + 'X-Requested-With': 'XMLHttpRequest' + } + } ) + .then( response => response.json() ) + .then( response => { + returnUrl = response.href; + } ); + + /* End of copied GitHub code */ + // Retrieve the target Amazon S3 upload URL. const uploadUrl = response.upload_url; From 4db90dd71dc58757feac652d439e519c085b4617 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Szcz=C4=99=C5=9Bniak?= Date: Tue, 6 Jun 2023 14:41:17 +0200 Subject: [PATCH 2/6] Code refactor. --- src/app/plugins/githubuploadadapter.js | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/app/plugins/githubuploadadapter.js b/src/app/plugins/githubuploadadapter.js index ede01ab..e539efd 100644 --- a/src/app/plugins/githubuploadadapter.js +++ b/src/app/plugins/githubuploadadapter.js @@ -103,26 +103,19 @@ export class Adapter { // Step 2: the real upload takes place this time to Amazon S3 servers, // using information returned from Step 1. - // The final URL of the file is already known, even before the upload. We save it here. - returnUrl = response.asset.href; - - /** - * This is a copy&paste code from gitHub source code `attachment-upload.ts` - * - * It marks uploaded file as completed and return correct URL. - */ - const url = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; - const token = typeof response.asset_upload_authenticity_token == 'string' ? + const assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; + const authenticityToken = typeof response.asset_upload_authenticity_token == 'string' ? response.asset_upload_authenticity_token : null; - if ( !( url && token ) ) { + if ( !( assetUrl && authenticityToken ) ) { return; } const form = new FormData(); - form.append( 'authenticity_token', token ); - fetch( url, { + form.append( 'authenticity_token', authenticityToken ); + + fetch( assetUrl, { method: 'PUT', body: form, credentials: 'same-origin', @@ -133,11 +126,10 @@ export class Adapter { } ) .then( response => response.json() ) .then( response => { + // The final URL of the file is already known, even before the upload. We save it here. returnUrl = response.href; } ); - /* End of copied GitHub code */ - // Retrieve the target Amazon S3 upload URL. const uploadUrl = response.upload_url; From 6815c2b31f2bd677c6064c5afbf6ba7a86f36df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Szcz=C4=99=C5=9Bniak?= Date: Fri, 9 Jun 2023 09:27:03 +0200 Subject: [PATCH 3/6] Review fixes. --- src/app/plugins/githubuploadadapter.js | 76 +++++++++++++++----------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/src/app/plugins/githubuploadadapter.js b/src/app/plugins/githubuploadadapter.js index e539efd..fdc87ed 100644 --- a/src/app/plugins/githubuploadadapter.js +++ b/src/app/plugins/githubuploadadapter.js @@ -52,8 +52,8 @@ export class Adapter { * @returns {Promise<{default: *}>} A promise that resolves to an object containing the url to reach the uploaded file. */ upload() { - // This variable holds the final result of the whole upload logic: the URL to the uploaded file. - let returnUrl; + // This variable holds response data from first asset upload ( to pass it to the `sendAssetRequest`). + let assetResponse; // This is an async operation that involves 2 or 3 xhr requests. // @@ -103,32 +103,8 @@ export class Adapter { // Step 2: the real upload takes place this time to Amazon S3 servers, // using information returned from Step 1. - const assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; - const authenticityToken = typeof response.asset_upload_authenticity_token == 'string' ? - response.asset_upload_authenticity_token : null; - - if ( !( assetUrl && authenticityToken ) ) { - return; - } - - const form = new FormData(); - - form.append( 'authenticity_token', authenticityToken ); - - fetch( assetUrl, { - method: 'PUT', - body: form, - credentials: 'same-origin', - headers: { - Accept: 'application/json', - 'X-Requested-With': 'XMLHttpRequest' - } - } ) - .then( response => response.json() ) - .then( response => { - // The final URL of the file is already known, even before the upload. We save it here. - returnUrl = response.href; - } ); + // Assign asset response to variable + assetResponse = response; // Retrieve the target Amazon S3 upload URL. const uploadUrl = response.upload_url; @@ -153,10 +129,14 @@ export class Adapter { this._sendRequest( data ); } ) ) .then( ( /* { file, response } */ ) => { - // Upload concluded! Simply send the file URL back, according to CKEditor specs. - return { - default: returnUrl - }; + // Step 3: Retrieve the final uploaded asset URL using response data gathered on Step 1. + return this.sendAssetRequest( assetResponse ) + .then( returnUrl => { + // Upload concluded! Simply send the file URL back, according to CKEditor specs. + return { + default: returnUrl + }; + } ); } ); } ); } @@ -236,4 +216,36 @@ export class Adapter { _sendRequest( data ) { this.xhr.send( data ); } + + /** + * Sends a request to receive a final asset URL. + */ + sendAssetRequest( response ) { + const assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; + const authenticityToken = typeof response.asset_upload_authenticity_token == 'string' ? + response.asset_upload_authenticity_token : null; + + if ( !( assetUrl && authenticityToken ) ) { + return; + } + + const form = new FormData(); + + form.append( 'authenticity_token', authenticityToken ); + + return fetch( assetUrl, { + method: 'PUT', + body: form, + credentials: 'same-origin', + headers: { + Accept: 'application/json', + 'X-Requested-With': 'XMLHttpRequest' + } + } ) + .then( response => response.json() ) + .then( response => { + // The final URL of the file is already known, even before the upload. We save it here. + return response.href; + } ); + } } From e10de92c8c0a87d647941e858b7a571b457ca5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Szcz=C4=99=C5=9Bniak?= Date: Fri, 9 Jun 2023 10:21:11 +0200 Subject: [PATCH 4/6] Firefox upload issue fix. --- src/app/plugins/githubuploadadapter.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/plugins/githubuploadadapter.js b/src/app/plugins/githubuploadadapter.js index fdc87ed..cf7e930 100644 --- a/src/app/plugins/githubuploadadapter.js +++ b/src/app/plugins/githubuploadadapter.js @@ -221,7 +221,7 @@ export class Adapter { * Sends a request to receive a final asset URL. */ sendAssetRequest( response ) { - const assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; + let assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; const authenticityToken = typeof response.asset_upload_authenticity_token == 'string' ? response.asset_upload_authenticity_token : null; @@ -229,6 +229,9 @@ export class Adapter { return; } + // Firefox fix + assetUrl = assetUrl.startsWith( 'https' ) ? assetUrl : `${ window.location.origin }${ assetUrl }`; + const form = new FormData(); form.append( 'authenticity_token', authenticityToken ); From 8e1c3b118652c454b571e6e0ba486cf71fc16ece Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 9 Jun 2023 12:33:01 +0200 Subject: [PATCH 5/6] Review fixes / simplifications. --- src/app/plugins/githubuploadadapter.js | 113 ++++++++++++------------- 1 file changed, 56 insertions(+), 57 deletions(-) diff --git a/src/app/plugins/githubuploadadapter.js b/src/app/plugins/githubuploadadapter.js index cf7e930..eac0074 100644 --- a/src/app/plugins/githubuploadadapter.js +++ b/src/app/plugins/githubuploadadapter.js @@ -52,9 +52,6 @@ export class Adapter { * @returns {Promise<{default: *}>} A promise that resolves to an object containing the url to reach the uploaded file. */ upload() { - // This variable holds response data from first asset upload ( to pass it to the `sendAssetRequest`). - let assetResponse; - // This is an async operation that involves 2 or 3 xhr requests. // // Start by taking the upload configuration, extracting it from the page. @@ -68,7 +65,15 @@ export class Adapter { return this.loader.file // Uploading on GH is made out of two steps. In our logic we're trying to mimic the requests that // the original GH pages do, including the exact sets of headers and data. - .then( file => new Promise( ( resolve, reject ) => { + .then( getAssetUploadPolicy.bind( this ) ) + // The file and the response from the above request are passed along. + .then( uploadFile.bind( this ) ) + // Finally we need to confirm the upload (GH added this feature in 2023). + // See https://github.com/ckeditor/github-writer/issues/444 for more details. + .then( sendAssetRequest.bind( this ) ); + + function getAssetUploadPolicy( file ) { + return new Promise( ( resolve, reject ) => { // Step 1: a setup request is made to the GH servers, returning the target upload URL // and authentication tokens. @@ -97,14 +102,13 @@ export class Adapter { // Run! this._sendRequest( data ); - } ) ) - // The file and the response from the above request are passed along. - .then( ( { file, response } ) => new Promise( ( resolve, reject ) => { - // Step 2: the real upload takes place this time to Amazon S3 servers, - // using information returned from Step 1. + } ); + } - // Assign asset response to variable - assetResponse = response; + function uploadFile( { file, response } ) { + return new Promise( ( resolve, reject ) => { + // Step 2: the real upload takes place this time to Amazon S3 servers, + // using information returned from asset policy at Step 1. // Retrieve the target Amazon S3 upload URL. const uploadUrl = response.upload_url; @@ -123,21 +127,51 @@ export class Adapter { this._initRequest( uploadUrl ); // _initListeners is the one responsible to resolve this promise. - this._initListeners( resolve, reject, file ); + this._initListeners( resolveParams => { + resolve( { + ...resolveParams, + assetPolicyResponse: response + } ); + }, reject, file ); // Run! this._sendRequest( data ); - } ) ) - .then( ( /* { file, response } */ ) => { - // Step 3: Retrieve the final uploaded asset URL using response data gathered on Step 1. - return this.sendAssetRequest( assetResponse ) - .then( returnUrl => { - // Upload concluded! Simply send the file URL back, according to CKEditor specs. - return { - default: returnUrl - }; - } ); } ); + } + + function sendAssetRequest( { /* file, response, */ assetPolicyResponse } ) { + let assetUrl = typeof assetPolicyResponse.asset_upload_url === 'string' ? assetPolicyResponse.asset_upload_url : null; + const authenticityToken = typeof assetPolicyResponse.asset_upload_authenticity_token == 'string' ? + assetPolicyResponse.asset_upload_authenticity_token : null; + + if ( !( assetUrl && authenticityToken ) ) { + return; + } + + // Firefox fix (https://github.com/ckeditor/github-writer/pull/450#issuecomment-1580669679). + assetUrl = assetUrl.startsWith( 'https' ) ? assetUrl : `${ window.location.origin }${ assetUrl }`; + + const form = new FormData(); + + form.append( 'authenticity_token', authenticityToken ); + + return fetch( assetUrl, { + method: 'PUT', + body: form, + credentials: 'same-origin', + headers: { + Accept: 'application/json', + 'X-Requested-With': 'XMLHttpRequest' + } + } ) + .then( response => response.json() ) + .then( response => { + // The final URL of the file is already known, even before the upload. We save it here. + return { + default: response.href + }; + } ); + } } ); } @@ -216,39 +250,4 @@ export class Adapter { _sendRequest( data ) { this.xhr.send( data ); } - - /** - * Sends a request to receive a final asset URL. - */ - sendAssetRequest( response ) { - let assetUrl = typeof response.asset_upload_url === 'string' ? response.asset_upload_url : null; - const authenticityToken = typeof response.asset_upload_authenticity_token == 'string' ? - response.asset_upload_authenticity_token : null; - - if ( !( assetUrl && authenticityToken ) ) { - return; - } - - // Firefox fix - assetUrl = assetUrl.startsWith( 'https' ) ? assetUrl : `${ window.location.origin }${ assetUrl }`; - - const form = new FormData(); - - form.append( 'authenticity_token', authenticityToken ); - - return fetch( assetUrl, { - method: 'PUT', - body: form, - credentials: 'same-origin', - headers: { - Accept: 'application/json', - 'X-Requested-With': 'XMLHttpRequest' - } - } ) - .then( response => response.json() ) - .then( response => { - // The final URL of the file is already known, even before the upload. We save it here. - return response.href; - } ); - } } From 7166deee8f33f86d33cb02538ac49e21eff061ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Szcz=C4=99=C5=9Bniak?= Date: Fri, 9 Jun 2023 15:32:21 +0200 Subject: [PATCH 6/6] Show images in Files in Code Review editor. --- src/app/theme/content.css | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/app/theme/content.css b/src/app/theme/content.css index df9149a..a4ca520 100644 --- a/src/app/theme/content.css +++ b/src/app/theme/content.css @@ -156,3 +156,16 @@ } } } + + +/* +https://github.com/ckeditor/github-writer/pull/450#issuecomment-1584553496 +https://github.com/ckeditor/github-writer/pull/450#issuecomment-1584573368 +*/ +.file.open { + & .image { + &.ck-widget { + display: block; + } + } +}