diff --git a/package.json b/package.json index bab3b790..99c92dfa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openstack-uicore-foundation", - "version": "5.0.50", + "version": "5.0.52-beta.0", "description": "ui reactjs components for openstack marketing site", "main": "lib/openstack-uicore-foundation.js", "scripts": { diff --git a/src/components/inputs/dropzone/__tests__/dropzone.test.js b/src/components/inputs/dropzone/__tests__/dropzone.test.js index 39ece6d3..9a1c2497 100644 --- a/src/components/inputs/dropzone/__tests__/dropzone.test.js +++ b/src/components/inputs/dropzone/__tests__/dropzone.test.js @@ -285,6 +285,236 @@ describe('DropzoneJS - HTTP 202 Polling UX', () => { }, 2500); }, 10); }, 10000); + /** + * Resolves a handler DropzoneJS registered on the underlying Dropzone instance, + * so a test can drive the real wiring instead of poking at internals. + */ + const getEventHandler = (instance, eventName) => { + const call = instance.dropzone.on.mock.calls + .slice() + .reverse() + .find(([evt]) => evt === eventName); + return call ? call[1] : null; + }; + + /** + * Test Case 6: cancelling a file stops ITS status polling - and only its own + * + * While the server processes an upload asynchronously (HTTP 202) the row sits in + * "Loading" with a delete button. Removing the file there used to leave the status + * interval running, so when processing finished the result was still pushed to the + * parent and the "cancelled" file ended up committed to the form. + */ + test('test_dropzone_cancel_stops_polling_for_that_file_only', (done) => { + // Server is still processing: every poll answers 'uploading', so polling keeps going. + global.fetch = jest.fn(() => + Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + ); + + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const fileA = { name: 'a.png', size: 1024 }; + const fileB = { name: 'b.png', size: 2048 }; + + instance.pollUploadStatus('file-a', 'https://example.com/upload', fileA); + instance.pollUploadStatus('file-b', 'https://example.com/upload', fileB); + + setTimeout(() => { + expect(global.fetch).toHaveBeenCalled(); + + const onRemovedFile = getEventHandler(instance, 'removedfile'); + expect(typeof onRemovedFile).toBe('function'); + onRemovedFile(fileA); + + const callsAtCancel = global.fetch.mock.calls.length; + + setTimeout(() => { + const urlsAfterCancel = global.fetch.mock.calls + .slice(callsAtCancel) + .map(([url]) => url); + + // fileB is untouched and keeps polling on its own interval... + expect(urlsAfterCancel.length).toBeGreaterThan(0); + // ...while the cancelled file never asks for its status again. + expect(urlsAfterCancel.some((url) => url.endsWith('/status/file-a'))).toBe(false); + + done(); + }, 2500); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 7: a status response that lands AFTER the cancel must not be committed + * + * Clearing the interval is not enough on its own: the tick that was already awaiting + * its status request resolves after the user cancelled, and used to fire the deferred + * success callback plus onUploadComplete. + */ + test('test_dropzone_cancel_while_status_request_in_flight_does_not_commit_result', (done) => { + let respondComplete = null; + global.fetch = jest.fn( + () => + new Promise((resolve) => { + respondComplete = () => + resolve({ + json: () => + Promise.resolve({ status: 'complete', name: 'test.pdf', size: 1024000 }) + }); + }) + ); + + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const chunksUploadedDone = jest.fn(); + const mockFile = { + name: 'test.pdf', + size: 1024000, + _asyncProcessing: true, + _chunksUploadedDone: chunksUploadedDone + }; + + instance.pollUploadStatus('file-123', 'https://example.com/upload', mockFile); + + setTimeout(() => { + // The first tick fired and is parked on the status request. + expect(typeof respondComplete).toBe('function'); + + // The user cancels while that request is still in flight... + getEventHandler(instance, 'removedfile')(mockFile); + // ...and only then does the server report the upload as processed. + respondComplete(); + + setTimeout(() => { + expect(chunksUploadedDone).not.toHaveBeenCalled(); + expect(onUploadCompleteMock).not.toHaveBeenCalled(); + done(); + }, 100); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 8: unmounting stops polling for EVERY file in flight + * + * The interval id used to live in a single component-level slot, so a second file + * starting to poll orphaned the first one's interval and it outlived the component. + */ + test('test_dropzone_unmount_stops_polling_for_every_file', (done) => { + global.fetch = jest.fn(() => + Promise.resolve({ json: () => Promise.resolve({ status: 'uploading' }) }) + ); + + const ref = React.createRef(); + + const { unmount } = render( + + ); + + setTimeout(() => { + const instance = ref.current; + + instance.pollUploadStatus('file-a', 'https://example.com/upload', { + name: 'a.png', + size: 1024 + }); + instance.pollUploadStatus('file-b', 'https://example.com/upload', { + name: 'b.png', + size: 2048 + }); + + setTimeout(() => { + expect(global.fetch).toHaveBeenCalled(); + + unmount(); + const callsAtUnmount = global.fetch.mock.calls.length; + + setTimeout(() => { + expect(global.fetch.mock.calls.length).toBe(callsAtUnmount); + done(); + }, 2500); + }, 2500); + }, 10); + }, 15000); + + /** + * Test Case 9: an upload response that arrives after the cancel must not be committed + * + * xhr.abort() on a request already in DONE state is a no-op, so the synchronous (200) + * path could still reach onUploadComplete for a file the user had just removed. + */ + test('test_dropzone_upload_response_after_cancel_does_not_commit_result', (done) => { + const ref = React.createRef(); + + render( + + ); + + setTimeout(() => { + const instance = ref.current; + const mockFile = { + name: 'test.pdf', + size: 1024000, + accessToken: 'mock-token', + md5: 'mock-md5' + }; + const mockXhr = { + readyState: XMLHttpRequest.DONE, + status: 200, + responseText: JSON.stringify({ + name: 'test.pdf', + path: 'uploads/', + size: 1024000 + }), + setRequestHeader: jest.fn(), + onload: jest.fn(), + onerror: jest.fn(), + abort: jest.fn() + }; + + // 'sending' is what installs the wrapper deciding what to do with the response. + getEventHandler(instance, 'sending')(mockFile, mockXhr, { append: jest.fn() }); + + // The user cancels; the response for the last chunk is already on its way back. + getEventHandler(instance, 'removedfile')(mockFile); + mockXhr.onload({}); + + expect(onUploadCompleteMock).not.toHaveBeenCalled(); + done(); + }, 10); + }); }); describe('DropzoneJS - Progress Bar Monotonicity', () => { diff --git a/src/components/inputs/dropzone/index.js b/src/components/inputs/dropzone/index.js index f41f9990..d619fc1e 100644 --- a/src/components/inputs/dropzone/index.js +++ b/src/components/inputs/dropzone/index.js @@ -22,6 +22,26 @@ export class DropzoneJS extends React.Component { this.activeXHRs = new Map(); // Track active XHR requests per file this.chunkQueue = []; this.chunksInFlight = 0; + // Status-poll interval ids, one per file in flight. Kept as a set (and mirrored on + // the file itself) rather than a single slot so a second file starting to poll + // cannot orphan the first one's interval. + this._pollIntervals = new Set(); + } + + /** + * Stops the status polling started by pollUploadStatus for this file, if any. + * Cancelling an upload has to reach the interval too: a file the user removed while the + * server was still processing it must stop asking for its status, otherwise the result + * lands later and gets committed as if the upload had been kept. + */ + stopPolling(file) { + if (!file) return; + if (file._pollIntervalId) { + clearInterval(file._pollIntervalId); + this._pollIntervals.delete(file._pollIntervalId); + file._pollIntervalId = null; + } + file._pollingActive = false; } onError(e, status){ @@ -77,12 +97,15 @@ export class DropzoneJS extends React.Component { const maxAttempts = 300; // 10 minutes at 2s intervals let attempts = 0; - this._pollInterval = setInterval(async () => { + const intervalId = setInterval(async () => { + // The file may have been removed since the last tick. + if (file._canceled) { + this.stopPolling(file); + return; + } attempts++; if (attempts > maxAttempts) { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError({ message: 'Upload timed out' }); return; } @@ -92,28 +115,32 @@ export class DropzoneJS extends React.Component { headers: { 'Authorization': `Bearer ${accessToken}` } }); const data = await response.json(); + // Clearing the interval is not enough on its own: this tick was already + // awaiting its response when the user cancelled, and committing it now + // would restore a file they removed. + if (file._canceled) { + this.stopPolling(file); + return; + } if (data.status === 'complete') { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); // Call the stored done callback to trigger Dropzone's success event if (file?._chunksUploadedDone) { file._chunksUploadedDone(); } this.onUploadComplete(data); } else if (data.status === 'error') { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError(data); } } catch (error) { - clearInterval(this._pollInterval); - this._pollInterval = null; - file._pollingActive = false; + this.stopPolling(file); this.onError(error); } }, 2000); + + file._pollIntervalId = intervalId; + this._pollIntervals.add(intervalId); } /** @@ -204,10 +231,8 @@ export class DropzoneJS extends React.Component { * Removes dropzone.js (and all its globals) if the component is being unmounted */ componentWillUnmount () { - if (this._pollInterval) { - clearInterval(this._pollInterval); - this._pollInterval = null; - } + this._pollIntervals.forEach(intervalId => clearInterval(intervalId)); + this._pollIntervals.clear(); // Clear chunk queue and cancel all pending XHR requests this.chunkQueue = []; @@ -343,6 +368,14 @@ export class DropzoneJS extends React.Component { this.dropzone.on('removedfile', (file) => { if (!file) return; + // Mark the file dead FIRST: both commit points (xhr.onload below and the status + // poll) check this flag, so a result that lands after the user cancelled is + // dropped instead of being pushed to the parent. + file._canceled = true; + this.stopPolling(file); + // A removed file must not fire Dropzone's deferred success event either. + file._chunksUploadedDone = null; + // Cancel all active XHR requests for this file const xhrs = this.activeXHRs.get(file); if (xhrs) { @@ -434,6 +467,13 @@ export class DropzoneJS extends React.Component { dropzoneOnLoad(e); + // The user may have cancelled while this response was in flight: abort() on an + // already-DONE xhr is a no-op, so without this check the result would still be + // committed for a file that is no longer in the list. 'canceled' is the value of + // Dropzone.CANCELED, compared as a literal so the guard does not depend on the + // Dropzone module being loaded. + if (file._canceled || file.status === 'canceled') return; + if(xhr?.status == 200) { if (typeof uploadResponse.name === 'string') { _this.onUploadComplete(uploadResponse); diff --git a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js index a855c3d6..59f3b058 100644 --- a/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js +++ b/src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js @@ -265,6 +265,53 @@ describe('UploadInputV3', () => { expect(container.querySelector('.dropzone-mock')).toBeVisible(); }); + test('does not show an error row when the user cancels the upload themselves', () => { + const removeFile = jest.fn(); + const dzFile = { name: 'sample.png', size: 11264 }; + + render(); + act(() => { + dropzoneCallbacks.onDropzoneReady({ files: [dzFile], removeFile }); + }); + act(() => { + dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); + }); + expect(screen.getByText(/Loading/)).toBeInTheDocument(); + + // The delete button on the uploading row is the only button on screen. + act(() => { + fireEvent.click(screen.getByRole('button')); + }); + expect(removeFile).toHaveBeenCalledWith(dzFile); + + // Dropzone reports a cancelled upload as an error carrying dictUploadCanceled. + act(() => { + dropzoneCallbacks.onFileError(dzFile, 'Upload canceled.'); + }); + + expect(screen.queryByText('Upload canceled.')).not.toBeInTheDocument(); + expect(screen.queryByText('sample.png')).not.toBeInTheDocument(); + }); + + test('does not show an error row for a file Dropzone reports as canceled', () => { + render(); + act(() => { + dropzoneCallbacks.onAddedFile({ name: 'sample.png', size: 11264 }); + }); + + // Cancelled by something other than the row's delete button (unmount, a consumer + // calling cancelUpload): Dropzone marks the file, our own flag is absent. + act(() => { + dropzoneCallbacks.onFileError( + { name: 'sample.png', size: 11264, status: 'canceled' }, + 'Upload canceled.' + ); + }); + + expect(screen.queryByText('Upload canceled.')).not.toBeInTheDocument(); + expect(screen.queryByText('sample.png')).not.toBeInTheDocument(); + }); + test('hides dropzone when an error is present', () => { const { container } = render(); act(() => { diff --git a/src/components/inputs/upload-input-v3/index.js b/src/components/inputs/upload-input-v3/index.js index 46a7766c..8e2d474c 100644 --- a/src/components/inputs/upload-input-v3/index.js +++ b/src/components/inputs/upload-input-v3/index.js @@ -211,6 +211,12 @@ const UploadInputV3 = ({ if (entry?.previewUrl) URL.revokeObjectURL(entry.previewUrl); return prev.filter(f => !(f.name === file.name && f.size === file.size)); }); + // Dropzone turns a cancelled upload into an error carrying dictUploadCanceled. A cancel + // is not a failure to report back to the user - the row just goes away. 'canceled' is the + // value of Dropzone.CANCELED, matched as a literal so this does not depend on the + // Dropzone module being loaded here; _userCanceled also covers files Dropzone never got + // to mark, such as one removed before its upload reached the UPLOADING state. + if (file._userCanceled || file.status === 'canceled') return; setErrorFiles(prev => [...prev, { name: file.name, size: file.size, message }]); }, []); @@ -232,7 +238,12 @@ const UploadInputV3 = ({ const dzFile = dropzoneInstanceRef.current.files?.find( f => f.name === file.name && f.size === file.size ); - if (dzFile) dropzoneInstanceRef.current.removeFile(dzFile); + if (dzFile) { + // Flag it before removing so handleFileError can tell this deliberate cancel + // apart from a real upload failure. + dzFile._userCanceled = true; + dropzoneInstanceRef.current.removeFile(dzFile); + } } }, []);